Core: Optimize utilization of global and window variables#813
Core: Optimize utilization of global and window variables#813leobalter wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
this is the most precise check I've got, we might want to simplify if to just check window.document.
There was a problem hiding this comment.
This seems like the wrong approach. We shouldn't alias global to window in the first place.
There was a problem hiding this comment.
It is different now and window is now renamed to global.
|
I also want to land another commit here in this PR renaming window to global to avoid this kind of confusion. |
There was a problem hiding this comment.
I get why we exclude the HTML reporter in node, but how does this make a difference?
There was a problem hiding this comment.
when QUnit is not exposed as a global value anymore, the following QUnit.difff line would fail.
There was a problem hiding this comment.
I see. I guess with ES6 modules this problem could go away? If so, let's extend the comment a bit and leave the if as-is.
There was a problem hiding this comment.
I've change the if to check this and also extended it to check if window has truth document property (avoiding Node apps with a window variable on the global).
|
@jzaefferer after a hard rebasing, I've fixed this branch with some more optimizations. The commits will be squashed using this (new) PR title. |
There was a problem hiding this comment.
This is wrong. It took me a while to understand the purpose. I'll remove this.
|
A few small issues left to address, overall looking good. |
There was a problem hiding this comment.
This block runs on every env and global is set to avoid errors on node.
Can you add a comment for that?
There was a problem hiding this comment.
updated to 1f91270 after rebase
Ref qunitjs#790 QUnit was found exported to the global object on Node environment
|
I rebased this branch with master and now it's crashing on the amd tests, I'll check and fix. |
|
Realized the error was on my machine only with a missing node module. |
|
Looks good to land. |
|
thanks! |
Ref #790
QUnit was found exported to the global object on Node environment.
This PR also removes QUnit.diff and QUnit.init functions on Node, they were always browser functions.
This might look as a breaking change, so we should also discuss if this if good to land only on 2.0 or maybe I could set a stub function for them printing a warning about their usage on non-browser envs, which is not functional.