Remove window object references and fix whitespace

[david-mark]

Summary

Checklist

Mark an [x] for completed items, if you're not sure leave them unchecked and we can assist.

• All authors have signed the CLA at https://contribute.jquery.com/CLA/
• New tests have been added to show the fix or feature works
• Grunt build and unit tests pass locally with these changes
• If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

Thanks! Bots and humans will be around shortly to check it out.

Always declare global variables.

https://gist.github.com/david-mark/7cf93afccf3696a4f6754b36cd6ab34e

[mention-bot]

@david-mark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @timmywil, @dmethvin and @mgol to be potential reviewers.

[jsf-clabot]

All committers have signed the CLA.

[david-mark]

Integration tests indicate that some other file needs a variable name change. Certainly "jQueryDefined" is defined in the file I changed. Also added it to "globals" in the lint JSON, which makes little sense to me as it isn't a global at all (nor is "noGlobal", which is also in there). Leave that to you.

Not sure what that "global.js" file is supposed to do (could use some more comments), but there's no good reason for it doing it the way it was.

Edit: I get it. It's strictly a NodeJS script. Had mistaken that one for some sort of AMD-like wrapper.

Edit: Quite the paradox: this "global.js" script is run in NodeJS and painted into a corner by the local "jQuery" argument. In NodeJS, could use "global" instead of "this", but the function is clearly not meant for NodeJS as it uses "window" to try to reference the global object.

There's only one quick way to fix this and that's to remove 'use strict'; unfortunately, that causes the integration test to fail for that file.

/home/travis/build/jquery/jquery/src/exports/global.js
3:4 error Use the function form of 'use strict' strict

Good luck with all that. Whatever you do, stop substituting the - window - host object for the global object. And ideally, declare all of your globals.

[timmywil]

Thank you for your contribution. However, we'd like to keep this the way it is. It has seen many revisions to get to this point (and has withstood the test of time). I see no practical reason to change it, just a blanketing ideological argument. Notice that the window variable used in a built jQuery is intentionally a shadowed var, which enables jQuery's use in Node when a window is mocked with something like jsdom (see https://github.com/jquery/jquery/blob/master/src/wrapper.js#L32 for what happens if you load jQuery in Node without a window). The window passed to jQuery is not always the same as the global object.

[david-mark]

Nothing ideological about the fact that host objects are not part of the language. Relying on them for no reason leads to confusion and cross-platform issues. The fact that the code seems to "work" is no reason to close this ticket and ignore the underlying confusion that likely led to the "many revisions". ;)

There is a very practical lesson to be learned here if you are willing to consider what led you to this point. Understanding the previous missteps surely trumps the "test of time". Just because it has somehow "worked" over an unspecified period of time does not mean there is nothing to be gained through understanding and correcting past mistakes.

Notice that the window variable used in a built jQuery is intentionally a shadowed var, which
enables jQuery's use in Node when a window is mocked with something like jsdom

That has nothing to do with my changes (though you likely need to make similar changes elsewhere to completely untangle this mess). You are simply confusing the window host object (and a surrogate provided for use in NodeJS) with the global object.

(see https://github.com/jquery/jquery/blob/master/src/wrapper.js#L32 for what happens if
you load jQuery in Node without a window).

Also has nothing to do with it. Did nothing to prevent the use of window. In Node the global object is named global and also has nothing to do with the DOM. That's a piece you are missing by conflating window and the global object in the various scripts that create the built jQuery. In NodeJS you reference the global object like this:

var global = global || this;

This has (or should have) nothing to do with window, regardless of what provides that object. Think about it and all of the pieces should fall into place. Or continue with the "show me where it fails" attitude and learn nothing.

Good luck! :)

[gibson042]

@david-mark Take another look at src/wrapper.js , and at uses of window throughout the codebase. We are not confusing window with the global object, we are explicitly depending upon a DOM-like window, complete with properties like document, location, addRemoveEventListener, getComputedStyle, requestAnimationFrame, and a self-referential window.

[david-mark]

Take another look at src/wrapper.js

I already saw it and just adjusted a line in my fork to support NodeJS, where the global object is named "global". As mentioned we were missing that bit as well. One more problem solved, but it had nothing to do with the rest of your post or the previous cite related to this file.

and at uses of window throughout the codebase. We are not confusing window with the
global object, we are explicitly depending upon a DOM-like window, complete with
properties like document, location, addRemoveEventListener, getComputedStyle,
requestAnimationFrame, and a self-referential window."

You are indeed confused.

You reference the global window object all over the built jQuery script (passed to your factory of course). In browsers, that's a property of the global object, so is in the global scope. As is document, which for some reason you reference as window.document. But that's beside the point as I changed none of those references. Seems a fair assumption that is done to support this jsdom object that is passed to the factory in NodeJS (more on that below).

And yes, the window object usually behaves like the global object, so it has a window property, as well as a window.window property and a window.window.window property, etc. Have no idea what you mean by "explicitly depending on a DOM-like window".

If you want to explicitly pass the global window object to your factory (which would make sense, but would not make a bit of difference in browsers), then do just that. Instead of checking for document as a property of window, check for global.window and global.document. Seems unlikely, but if CommonJS (or whatever) requires it, check for global.window.document instead of the latter (will also work fine for browsers).

In any event, it made no sense to pass window and fall back to this when absent. That's the conflation I untangled. Also note that the reference passed ends up as an argument called global, which now makes perfect sense.

As for whatever object gets passed to your factory when running in NodeJS, I can't see how that has anything to do with any of my changes, certainly not those to the wrapper.js file.

Per the comments, this is the pertinent part of the code:

// For CommonJS and CommonJS-like environments where a proper `window`
// is present, execute the factory and get jQuery.
// For environments that do not have a `window` with a `document`
// (such as Node.js), expose a factory as module.exports.
// This accentuates the need for the creation of a real `window`.
// e.g. var jQuery = require("jquery")(window);
// See ticket #14549 for more info.
module.exports = global.document ?
	factory( global, true ) :
	function( w ) {
		if ( !w.document ) {
			throw new Error( "jQuery requires a window with a document" );
		}
		return factory( w );
	};

And the previously cited line that throws the exception is inside your exports function. Haven't made any changes that affect that as it has its own w argument for NodeJS users to pass whatever object they want.

So what are you trying to say? Which line do you think will fail here and how do you figure my proposed changes will make it fail. Suggest removing the strict mode check from this file, allowing the integration tests to finish and then trying out the changes. Will likely save time.

Finally, this is just one change among several. There was at least one other place where window was used as a global object surrogate. Something to do with RequireJS, IIRC.

Best of luck! :)

PS. You really shouldn't use window.getComputedStyle. The proper reference is document.defaultView.getComputedStyle.

[david-mark]

Here is the latest pull request (now closed due to above confusion).

#3421

...As mentioned, the last change may not even be necessary, but certainly makes sense for NodeJS in case there is a way to provide global window and document objects without using a factory. As it stands, per the comments, the factory will always be used for NodeJS, so it really doesn't matter at this time.

[gibson042]

You reference the global window object all over the built jQuery script. In browsers, that's a property of the global object, so is in the global scope. As is document, but for some reason you reference that as window.document (but that's entirely beside the point).

No, we reference the local window that is the first parameter of the anonymous function referenced as factory in wrapper.js (the function that literally wraps our entire library).

Have no idea what you mean by "explicitly depending on a DOM-like window".

To repeat myself, "complete with properties like document, location, addRemoveEventListener, getComputedStyle, requestAnimationFrame, and a self-referential window". Or put another way, implementing the interfaces in the DOM Standard, HTML Standard, etc.—specifically Window itself.

And the previously cited line that throws the exception is inside your exports function.

Previously cited? Did I miss something?

So what are you trying to say? Which line do you think will fail here and how do you figure my proposed changes will make it fail.

The current code prioritizes window over the global object, so your change will hand some environments a factory where they currently receive the jQuery constructor. And your introduction of this references into the exports function will similarly fail to add jQuery and $ properties to the window object passed to the factory. But maybe I'm misunderstanding what you were going for... there's no issue reference and no unit tests.

PS. You really shouldn't use window.getComputedStyle. The proper reference is document.defaultView.getComputedStyle.

e488d98

[david-mark]

No, we reference the local window that is the first parameter of the anonymous function
referenced as factory in wrapper.js (the function that literally wraps our entire library).

I had just edited my comment to make that more clear, but it's still a reference to the global window object, at least in browsers. I changed it to a reference to the global object in all environments, whereas you were passing window in browsers (and CommonJS?) and this in other environments (e.g. NodeJS), which made no sense (e.g. this is a reference to the module in NodeJS).

To repeat myself, "complete with properties like document, location,
addRemoveEventListener, getComputedStyle, requestAnimationFrame, and a self-referential > window". Or put another way, implementing the interfaces in the DOM Standard, HTML
Standard, etc.—specifically Window itself.

Repeating it doesn't make it any more lucid. Re-read what I posted, particularly after the last edit. You do realize that both window and document are globals in browsers? And I gave you some additional tips on how to deal with other environments.

Previously cited? Did I miss something?

Apparently. Cited by "timmywil" above.

The current code prioritizes window over the global object, so your change will hand some
environments a factory where they currently receive the jQuery constructor.

There's just no need to prioritize anything. Re-read the suggestions I made in the last edit. In short, you start by passing a reference to the global object (as an argument that has always been called "global"). From there you can find both window and document in any environment. That's clear, whereas passing either window or this to the anonymous function is not (see previous post for a more detailed explanation).

And your introduction of this references into the exports function will similarly fail to add
jQuery and $ properties to the window object passed to the factory. But maybe I'm
misunderstanding what you were going for... there's no issue reference and no unit tests.

This is another misunderstanding. You should not be adding jQuery and $ to the window object at all, but to the global object. If you expect them to be on the window object elsewhere in the project in environments where that will make a difference, that is just another bit to untangle. Eventually it will all make sense... or not. Am growing tired a bit tired of this. :(

How can there be an issue reference is there is no known bug? This is just about untangling a mess and making the code that much easier to follow. And unit tests will be relevant once the integration tests are adjusted to allow the removal of the useless 'use strict' directive in one of the other files. Existing unit tests of course.

Again, good luck to you!

[david-mark]

New pull request:

#3422

Read carefully before closing and perhaps you won't close this one. I agree that there was some fine-tuning required to be perfect. Hadn't claimed perfection in the first place, but insist that the end result of my efforts is much clearer code. Will add some comments next to be perfectly clear about what it is doing and why.