make console.log available for logging statements in node environment

[bgmort]

In a node environment, this is not the global object in the scope of a module. So when it is passed as root, there is no console.log present on in, so log statements are never printed. This pull request passes the correct global object in any context, fixing logging statements when running under node.

[jimmyhchan]

Can you please add a test case to show how this is broken?

Steps to reproduce and node version also please. I thought we had tests
showing this working correctly.

Thanks

[lalitkapoor]

If I'm not mistaken this is the global object in the scope of a module. a simple check in the node repl shows that this === global is true unless I'm misunderstanding something here.

[bgmort]

That's true. In the repl, this === global. But in a module (including a top level module), that's not the case. this is just an a plain object. I'm not sure why that's the case, or if it was different in the past, but I verified it in both 0.8.25 and 0.10.26 before submitting the pull request. I couldn't see tests to verify logging working, but I may have overlooked them. I will try to add tests if I can find the time. If I do, where should they go? I ran the built-in tests; they only seemed to be running against phantom and rhino.

[bgmort]

I found the answer to my question about node tests in the Travis CI test results -- there's a separate testNode grunt task. If you compare the output of the test following my commit to the ones before it, it's quite clear that it fixes the logging because it starts showing up in the test output. Compare https://travis-ci.org/linkedin/dustjs/jobs/21213485 and https://travis-ci.org/linkedin/dustjs/jobs/20245030.

[jimmyhchan]

Thanks for looking into this and thanks for the link. It seems we have confused root/window/global/exports. I'm slightly hesitant to pull this change as is since I believe this becomes window/global instead of window/exports. I'm not sure though.

Some alternatives: since this appears to only affect console, what do you think about keeping this and changing the conditional to (console && console.log) instead?

separate topic: @prashn64, since debugging is none by default should we even be seeing the logging?

[bgmort]

console && console.log would throw a reference error when console is undefined (old IE). You'd still want to check root.console, or use typeof console !== 'undefined'. But considering that the variable is called 'root', it seems that you do want it to be window/global and not window/exports. The only other reference to root looks like this:

if (typeof exports === 'object') {
  module.exports = dust;
} else {
  root.dust = dust;
}

so right now you aren't making any assumptions about root being equal to exports.

[prashn64]

The logs are showing up in Travis because they are turned on when testing the debugger.

As for this PR, it looks compelling, but I'll have to look at it more closely this weekend.

[jimmyhchan]

Since we are using exports directly it does seem like this is the right
direction.

I was hoping to find some discussion in the umdjs patterns repo but haven't
found anything yet.

[prashn64]

It seems like testNode is missing from the local "grunt test" task, and is only tested in travis. I think the best way to add tests for this PR is to add grunt testNode to grunt test if that is possible.

[kate2753]

shell:oldTests is the task to run unit tests in node. It is run as part of grunt test

[jimmyhchan]

@prashn64 thoughts? i vote 👍 pull.

[jimmyhchan]

thanks @bgmort