remove window

[ganarajpr]

Assuming the existence of window means redux-logger cannot be used in universal applications.

This removes the dependency on window being the global variable.

[yocontra]

+1

[imevro]

It breaks default behavior. Pass console to logger option.

[ganarajpr]

I am not sure what default behaviour it breaks. console exists in both browser and nodejs globals. Could you explain ? Using the logger option is not a possibility since this breaks at import - a work around is to do perhaps a conditional import - but that just looks like a mess.

The only thing this removes is an assumption that the global is called window. Could you rationalize why you rejected this PR ?

[yocontra]

Uh... what? How does this break any behavior?

[gaearon]

Unless I'm missing something this should fix Node usage and should not have any effect in the browser. Any downsides?

[gaearon]

Since we are officially recommending this library on the docs, and people are using Redux on server, we would prefer to see this fixed.

[imevro]

#12

[esamattis]

It's because the variable is also named console and it shadows the global console object. Rename the console variable or use typeof to check for the window global.

Here's example of what was happening https://jsfiddle.net/Lzmuftt4/

[imevro]

@epeli no, see screenshot.

@gaearon why does middleware lose console on the third function in carry-functions? On 21 and 23 lines everything ok, but 25 is failed when tried to access global console instead of window.console.
I installed [email protected] with this bug.

[esamattis]

I think it's because the var console is being hoisted. The code in the screenshot is same as this:

return function (action) {
  var console; // make the console undefined
  console.log(console);
  var collapsed = options.collapsed;
  var predicate = options.predicate;
  var logger = options.logger;

  console.log(console);
  console = logger || console;
  console.log(console);

..

[imevro]

@epeli it doesn't have an explicit declaration.

https://github.com/fcomb/redux-logger/blob/d096ffb529881b62f2a5191d20a06fd4a5e6242a/build/createLogger.js (1.0.2)

[esamattis]

It doesn't need to have. That's the sad magic of var hoisting in Javascript :(

Here's a fiddle demonstrating it https://jsfiddle.net/Lzmuftt4/2/

[imevro]

@epeli oh wow! But how we can fix it? I don't have an idea.

[esamattis]

Hmm. In the current version there is no more var console. I think you can just merge this PR without any downsides. Or did you see the error with this one too?

[imevro]

@epeli yeah, you are right. Thanks, @ganarajpr @contra @gaearon @epeli!

[imevro]

Released 2.5.0.

[gaearon]

Thank you for getting back to this quickly!

[esamattis]

I think here are some old lessons to be learnt again.

I'm gonna enable "no-redeclare": [2, {"builtinGlobals": true}] in my projects...