Skip to content

make logging optional for webgl renderer#5835

Closed
JackCA wants to merge 1 commit intomrdoob:masterfrom
JackCA:webgl-renderer-logging-parameter
Closed

make logging optional for webgl renderer#5835
JackCA wants to merge 1 commit intomrdoob:masterfrom
JackCA:webgl-renderer-logging-parameter

Conversation

@JackCA
Copy link
Copy Markdown

@JackCA JackCA commented Dec 30, 2014

Adds a logging parameter to the WebGL renderer.

Useful for situations where you don't want logging at all such as production or in a testing environment. Retains true as default but would think falsewould be a better option?

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Dec 31, 2014

Why would one not want logging on production?

@bhouston
Copy link
Copy Markdown
Contributor

We could combine into this: #5791 -- Just add another THREE.log(). It would feed all through the same system.

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Dec 31, 2014

@mrdoob the production case for this change might not be as strong as the test one (which would be that there is test pollution any time a renderer is initialized) -- but I believe the strongest case for prod is that no other JS framework that I've used/seen automatically logs out purely informational content like this without allowing you to turn it off?

Regardless, the test situation is much more relevant for test runners that will log out things like this.

Aside: loving this library and the work you've done 🤘

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Dec 31, 2014

@bhouston I definitely think that is the right direction! If you implemented that log handler then I would need to no-op on it in my prod and test environments to get the behavior I want.

Ideally, one could set a global THREE.logLevel or something of that nature that would behave the same way as this PR. I wasn't sure if I missed that when looking over your source.

bhouston added a commit to bhouston/three.js that referenced this pull request Dec 31, 2014
@bhouston
Copy link
Copy Markdown
Contributor

@JackCA I've made the change on my PR (and simplified the log handlers too.) You can yourself add listeners to THREE.warning, THREE.error and THREE.log to add a logLevel-like handler. Would that satisfy your needs? I'm concerned about adding too much logging infrastructure to THREE.js as @mrdoob makes a lot of effort to minimize the code ThreeJS size.

BTW if you support my PR, please test it and inform me on the PR if it works for your needs. The more in support of the PR, the more likely it is to get merged. :)

@WestLangley
Copy link
Copy Markdown
Collaborator

Personally, I would prefer that this single line remain required.

console.log( 'THREE.WebGLRenderer', THREE.REVISION );

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Dec 31, 2014

Yeah, me too... That line is very useful when helping people. If we allow them to hide that message and they have some iframe or require.js business going there is no way to know what version they're using.

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Dec 31, 2014

@mrdoob I'm not sure what situation someone would be in where they consciously disable logging, yet don't know what version of three.js they're using...

I've never seen mandatory logging like this before and I think there is a lot of precedence and good reasons for making it optional, but if you're against that, then feel free to close this PR.

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Dec 31, 2014

Oh. You'll be surprised. Humans are very unreliable.

I've never seen mandatory logging like this before and I think there is a lot of precedence and good reasons for making it optional

Do you know any articles on the subject?

bhouston added a commit to bhouston/three.js that referenced this pull request Dec 31, 2014
This reverts commit 632002c.

Conflicts:
	src/Three.js
@bhouston
Copy link
Copy Markdown
Contributor

The king of logging frameworks is log4j (4j = For Java) which has evolved into the Apache Logging System: http://logging.apache.org/

Architecture here: http://logging.apache.org/log4j/2.x/manual/architecture.html

The JavaScript / NPM version is here: https://github.com/nomiddlename/log4js-node

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Dec 31, 2014

@mrdoob I don't have any articles off hand -- but just look at angular, ember, jQuery, etc. None of them have a mandatory log out to the console anywhere. Why would Three.js be any different? Those libraries have more users, more use-cases, more versions, and more that can go wrong, yet they get away with it.

I'm a fan of how ember does it: http://emberjs.com/guides/understanding-ember/debugging -- they're using something similar to what @bhouston did. Also see generic solutions such as: https://github.com/pimterry/loglevel.

I'm sure you've run into a lot of situations where people don't know what version they're using and don't know how to run THREE.revision in the console (or change the console context to the iframe situation you mention) and can empathize with that.

But I'm not even suggesting you remove it -- I've retained the current functionality where the default setting is to still log out the version number.

I think @bhouston is on the right track with the logging wrappers, but with your recent revert on the change that was relevant to this PR, I can't suggest it for my purposes -- it should be all or nothing.

My use case is purely for integration testing: I'm using dozens and dozens of libraries and the only thing polluting my test runners output is this one line.

Think it's an anti-pattern to be noisy regardless of the desire of the developer which is why you don't see jQuery: version 2.1.1 logged out in any production site out there.

@WestLangley
Copy link
Copy Markdown
Collaborator

That line is very useful when helping people.

It is also useful in seeing where three.js is being used on the net.

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Dec 31, 2014

@WestLangley you can find that out via the existence of THREE as a global variable -- and that hardly seems like a compelling reason for why it should be mandatory.

@crobi
Copy link
Copy Markdown
Contributor

crobi commented Jan 2, 2015

That line is very useful when helping people. It is also useful in seeing where three.js is being used on the net.

In addition to what @JackCA said, both of the above require the user to open the development console and are therefore only useful to developers. Such users probably know how to type THREE.REVISION in the console, so it is only very a minor convenience.

If you want to see where three.js is used (and get some publicity), wouldn't it be better to create a nice, official icon and suggest people put it on their website? That's what I've seen with other libraries like those @JackCA mentioned in his previous post.

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Jan 2, 2015

If you want to see where three.js is used (and get some publicity), wouldn't it be better to create a nice, official icon and suggest people put it on their website?

This is not about publicity...

@WestLangley
Copy link
Copy Markdown
Collaborator

you can find that out via the existence of THREE as a global variable -- and that hardly seems like a compelling reason for why it should be mandatory.

How would you do that in this jsfiddle, for example?

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Jan 2, 2015

@WestLangley
screen shot 2015-01-02 at 9 46 27 am

Notice the context of the debugger is changed to the result of the jsfiddle

@bhouston
Copy link
Copy Markdown
Contributor

bhouston commented Jan 2, 2015

@JackCA: @mrdoob and @WestLangley are right that in complex applications, it can be hard to find the THREE variable. I know if you are using a module-based system (such as RequireJS or similar, e.g. https://www.npmjs.com/package/three), THREE won't be a first-level global variable but may be arbitrarily nested somewhere.

@WestLangley
Copy link
Copy Markdown
Collaborator

@JackCA Thank you for the jsfiddle solution. : - )

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Jan 2, 2015

@bhouston If someone is using a relatively complex system like RequireJS and they don't know how to figure out what version of three.js they are including, I think they've got bigger problems...

Regardless: I'm trying to see if there is a compelling reason for why it should be mandatory to log this out. Everyone has chimed in on why it's nice to have/helpful in the first place, but not why we shouldn't be able to silence it if desired.

@bhouston
Copy link
Copy Markdown
Contributor

bhouston commented Jan 2, 2015

@JackCA We have a complex unit and integration test framework and we've never had an issue with unwanted console.log output -- we check for specific things in our tests and ignore console.log completely. This generally works for us. Thus while it may be a bit cleaner to be able to redirect ThreeJS's console.log output, I can not see why it MUST be redirected, it seems like an aesthetic choice either way.

Generally I am not familiar with testing frameworks that look at console.log and depend on it to be perfectly clean. In fact I am usually used to adding console.log/console.trace statements and other messy things while debugging failing unit tests

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Jan 2, 2015

@bhouston my testing framework/runner doesn't depend on a clean output -- it's a matter of having a clean visual output at the end of the test. If something goes wrong, then I expect to start seeing some logging and traces.

It's definitely a matter of aesthetics but I know that it is a common practice to not expect application logic to cause logging during tests (especially during passing tests). I think the term "test pollution" refers directly to this type of thing.

@titansoftime
Copy link
Copy Markdown
Contributor

but not why we shouldn't be able to silence it if desired.

I think a good reason why this should not be removed is the fact that three.js should get credit for their library. Three doesn't require us to put a "Powered by Three.js" or some link on our projects, the least we could do is leave it in the console.

If it is too bothersome, why not simply comment out the line in src?

@JackCA
Copy link
Copy Markdown
Author

JackCA commented Jan 8, 2015

I think a good reason why this should not be removed is the fact that three.js should get credit for their library

This is absolutely an awesome library and definitely deserves some credit, but quoting @mrdoob earlier:

This is not about publicity...

And your other question:

If it is too bothersome, why not simply comment out the line in src?

I use bower so I wouldn't be able to do that cleanly.

@octalmage
Copy link
Copy Markdown

I just want to add that I've never seen a production JavaScript library produce any output by default (without debug enabled).

That said, it's obviously not a big deal. I really don't care either way!

📟

@mrdoob mrdoob closed this Mar 15, 2015
@Qix-
Copy link
Copy Markdown

Qix- commented Jan 14, 2019

Do we prefer better quality software or a clean console?

Both. You ask your users to provide enough debugging information when asking for help, just like good engineers should be doing. You can only help those that want to help themselves.

@cyberhck
Copy link
Copy Markdown

looking at console isn't the only way of determining version, is it? why not check in package.json? or just export a global variable called THREE_VERSION or something like that and let them give you version that way.

Or I could argue with having side effects, logging would be a side effect, and should be avoided, just log version when I ask it to log, say THREE.printVersion() or something like that, but it's doing something user hasn't explicitly asked to do. Extension approach sounds better for me.

@Ontopic
Copy link
Copy Markdown

Ontopic commented Feb 25, 2019

I've been working a long long time with WebGL, but mostly vanilla or with Regl. Now I had the chance to start some client work with Three.js. The client is quite tech savvy and checked the console for his new website. The console message was unacceptable to them, so I had to monkey patch the console object to prevent the message from appearing (which makes me feel dirty 😿).

I understand the argument for maintainers sake, but from a business perspective this PR should have landed a long time ago. Still, big thank you to all the maintainers, I've never been able to get out impressive WebGL stuff so quickly!

@cyberhck
Copy link
Copy Markdown

actually not only version, but webgl could have a logger, and it could default to verbose, but as developers, if we could set the log level to error, and hide all the info and warnings in production, that'd be great.

@fiso
Copy link
Copy Markdown

fiso commented Apr 7, 2019

This discussion seems needlessly stubborn, can we PLEASE just have that one line line be optional? Make it enabled by default if you must, but PLEASE make it optional. 🙏

@fiso
Copy link
Copy Markdown

fiso commented Apr 7, 2019

Anyway here's a little helper for anyone else who's annoyed by this.

const filterLogging = (undesiredPattern, code) => {
  const originalLog = console.log;
  console.log = function (...args) {
    return originalLog(...args && args.join(' ').match(undesiredPattern) ? args : []);
  };
  code();
  console.log = originalLog;
};

Usage:

    filterLogging(/^THREE\.WebGLRenderer\s*\d*$/, () => {
      this.renderer = new THREE.WebGLRenderer();
    });

@sebamarynissen
Copy link
Copy Markdown

I modify the build itself - though I know this isn't good practice and I need to do it again once I update to another version - by wrapping up the code like this

(function(console) {console.log = function() {};

// Code from build/three.js

})(Object.create(console));

I can imagine that using build systems like webpack this line could be added programmatically.

@ghost
Copy link
Copy Markdown

ghost commented Aug 5, 2019

Since this is a closed pull request and won't be reopened, where is the issue that is handling this problem? I've read most of this discussion and there are a lot of reasons for making it optional, and there are a lot of good ideas for fixing it, but nothing seems to be happening with it.

It was suggested to open a new issue, but any attempts for new issues or new PRs just get marked as duplicates of this PR (here, here, here, here, here), which is closed.

So what are the next steps? I'd like to get the single console log of the version removed in my own website (none of my other libraries have any required logs).

IMO, I feel that any website that uses ThreeJS (or any library) should have control over the console logging, and not the library itself, since the developers of the website know best what their clients want. In this particular case, perhaps the website should simply be able to query ThreeJS for the version and console log if it wants. Something as simple as threeJsRenderer.getVersion() instead of the console.log.

@dmarcos
Copy link
Copy Markdown
Contributor

dmarcos commented Aug 5, 2019

@stephenhurleyjpl wrap console.log and filter out THREE output as described in #5835 (comment)

@fiddleplum
Copy link
Copy Markdown

@dmarcos I'll do that as a temporary solution, but that is just a band-aid hack (modifying console.log) until a proper solution is found. Just imagine if modifying the built-in console.log was required by everyone's library for removing all output in production code. Still waiting on a response for how to move this issue forward.

@arpu
Copy link
Copy Markdown

arpu commented Aug 5, 2019

if you use webpack you can use the terser plugin

minimizer: [
      new TerserPlugin({
        extractComments: true,
        cache: true,
        parallel: true,
        sourceMap: false, // Must be set to true if using source-maps in production
        terserOptions: {
          // https://github.com/webpack-contrib/terser-webpack-plugin#terseroptions
           extractComments: 'all',
           compress: {
               drop_console: true,
           },
        }
      }),
    ]

@ghost
Copy link
Copy Markdown

ghost commented Aug 5, 2019

@dmarcos Sorry, I commented to you on my other account accidentally, but I'm the same person as @fiddleplum (work vs home).

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Aug 5, 2019

Console.log removed. 1013fbb

@ghost
Copy link
Copy Markdown

ghost commented Aug 5, 2019

Console.log removed. 1013fbb

Thanks @mrdoob! :D

@andrevenancio
Copy link
Copy Markdown
Contributor

unsubscribe.

@fiso
Copy link
Copy Markdown

fiso commented Aug 6, 2019

Finally, some closure! 🙏

@jonny-improbable
Copy link
Copy Markdown

Argh, all of my tests are now failing because of this breaking change!

@Mugen87
Copy link
Copy Markdown
Collaborator

Mugen87 commented Aug 13, 2019

It seems no matter what we do with this issue, someone will always complain 😞

@Qix-
Copy link
Copy Markdown

Qix- commented Aug 13, 2019

@jonny-improbable You're over-testing, then. That is not Three.js's problem. I suggest researching how to do proper testing. Google is a great resource.

This issue should probably be locked as "Resolved".

@cyberhck
Copy link
Copy Markdown

How would this be a breaking change? You probably should test better.

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Aug 13, 2019

@jonny-improbable can you elaborate on how removing a console.log is affecting your tests?

@philipisapain
Copy link
Copy Markdown

Wait... @jonny-improbable, are you suggesting that asserting a third-party library calling console.log is not a valid test?

@Rcreators

This comment has been minimized.

@Qix-

This comment has been minimized.

@funwithtriangles
Copy link
Copy Markdown
Contributor

Has setting a global variable been considered to allow curious devs to inspect the version number of other projects? Also I'm interested to know why the implementation hasn't been made optional like the original PR?

@Qix-
Copy link
Copy Markdown

Qix- commented Sep 8, 2019

Because browser APIs aren't designed with "curious devs" in mind, but instead the actual devs. If everything was reverse engineer friendly, there'd be a lot of extra cruft in the world.

@WestLangley
Copy link
Copy Markdown
Collaborator

I'm interested to know why the implementation hasn't been made optional like the original PR?

I wish it had been.

@mrdoob
Copy link
Copy Markdown
Owner

mrdoob commented Sep 9, 2019

@funwithtriangles we're moving to devtools: https://github.com/threejs/three-devtools

But, we forgot to expose the revision in r108, should be fixed in r109: #17342

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.