Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support Node-like module loaders #1103

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

isaacs commented Dec 26, 2012

Set module.exports to the jQuery object in Node, and other module loaders like it.

Do not unnecessarily pollute the global namespace in AMD/Node/CommonJS module loaders.

@rwaldron rwaldron commented on an outdated diff Dec 26, 2012

src/exports.js
@@ -16,3 +13,14 @@ window.jQuery = window.$ = jQuery;
if ( typeof define === "function" && define.amd && define.amd.jQuery ) {
define( "jquery", [], function () { return jQuery; } );
}
+
+// Expose jQuery as module.exports in loaders that implement the Node
+// module pattern (including browserify)
+else if ( typeof module === "object" && typeof module.exports === "object" ) {
+ module.exports = jQuery;
+}
+
+// Otherwise expose jQuery to the global object as usual
+else {
+ window.jQuery = window.$ = jQuery;
@rwaldron

rwaldron Dec 26, 2012

Member

Indents need to be tabs, not spaces

Member

rwaldron commented Dec 26, 2012

The else statements should start on the same line as the preceeding IfStatement's closing curly, with the comments immediately inside the block. Thanks :)

Contributor

isaacs commented Dec 26, 2012

Style fixups squashed into 90cc01f5bf3d360783f88daf56000e94b0f6eae2.

Member

Krinkle commented Dec 31, 2012

+1

@gibson042 gibson042 referenced this pull request in jquery/sizzle Jan 3, 2013

Closed

Added commonjs type exports #183

Owner

dmethvin commented Jan 20, 2013

Where does this pull request stand? @rwldrn did you have other concerns?

Contributor

dcherman commented Jan 21, 2013

Is there a good reason to not expose jQuery/$ as a global when an AMD loader is present like the current PR does? That's a breaking change for AMD users FWIW.

For comparison, other AMD compatible utilities like Lodash, Hogan.js, and the Backbone/Underscore AMD versions maintained by @jrburke still create a global even in the presence of an AMD loader.

Contributor

jrburke commented Jan 21, 2013

Right, the global should still be exposed at least in the AMD case. Probably for the commonjs style case if they expect to run in the browser. As we have seen with other base libraries that support "plugins", like backbone and underscore, those plugins normally assume a global for the base library and attach themselves to it. They are slower to adopt a module registration pattern.

domenic commented Jan 28, 2013

Ping @isaacs; up for incorporating the above concerns? I can do it instead.

Contributor

isaacs commented Jan 28, 2013

So, the consensus is that in the AMD case, it should still expose a global? I have no opinions there, so I'm happy to change it.

In the node-like case, it should definitely not create a global. That's considered bad behavior.

domenic commented Jan 28, 2013

But in the Node-like case, it's going to be used in the browser, and if we want to be able to use it along with jQuery plugins, it needs to create a global :-/.

Contributor

isaacs commented Jan 28, 2013

Really? In the browser, there's going to be a module object with an exports member?

What node-like module loaders are there in the browser that depend on globals being created? Browserify certainly doesn't.

Contributor

jrburke commented Jan 28, 2013

It would be good to understand the actual use case for needing this. This does not seem to be Node-like, but more CommonJS-like browser loaders. I thought the trend in the folks that liked those loaders was to always do builds. And in that case, it seems fine to just suggest those loaders use AMD as their transport format.

Contributor

isaacs commented Jan 28, 2013

@jrburke People are using jquery in Node today. But it's a huge confusing pita, because npm install jquery is not jQuery, it's a jsdom thing, and out of date. (jQuery has some things that are not browser-specific, I guess? I don't know, it seems silly to me, but whatever, people do silly things sometimes and it's fine :)

If someone builds jQuery using a tool that presents a Node module interface, then that tool must not be depending on leaking globals, since this is simply not done in the Node module world. Whether it uses an AMD or AMD-like transport mechanism, the API provided to the user clearly does not depend on leaking globals. What the user will expect is that var $ = require('jquery') loads the jQuery object into their local $ var, and does not create any globals whatsoever.

This does not seem to be Node-like, but more CommonJS-like browser loaders.

CommonJS is dead. Let's be honest here, there only "CommonJS" module system in use in any relevant way is Node, which never implemented more than the barest minimum of the CommonJS spec, and then went wildly off the rails as that organization designed-by-committee itself into obsolescence years ago. And the only build tool that we're actually talking about is Browserify, which presents a Node module interface for use on the browser, by generating AMD-like boilerplate that it wraps around the code.

domenic commented Jan 28, 2013

The use case here is npm + browserify. Simple as that.

With that in mind, leaking globals is desired, because you might use npm + browserify+ jQuery + a jQuery plugin, and jQuery plugins depend on the jQuery global.

There is a difference in "what is done" in the browserify world and in the pure Node.js world. In the browserify world, globals are still a fact of life, and must be accomodated---especially for packages like jQuery that have a plugin architecture dependent entirely on the global namespace.

Put another way, what the user expects is that not only does var $ = require("jquery") load the jquery function, but also that <script src="jquery.color.js"></script> works on the same page.

Contributor

jrburke commented Jan 28, 2013

@isaacs: yeah, the "npm in jquery" points to not putting front end code in npm. While not having globals would be great for a module system in the browser, it turns out is a huge support headache since module support in client code is so uneven.

So this change seems unnecessary. Browserify would do better by just updating its wrapping to AMD. We don't need to have more format wars around modules. Node's module system is not a good fit for direct use in the browser, and there will always need to be a bundled format for use in the browser. For that, AMD has the mindshare. Let's move on to solving higher level problems, or helping ES to get it right.

domenic commented Jan 28, 2013

I strongly disagree, @jrburke. We have used npm packages and node modules with great success in the browser at multiple companies and in several projects. Module support in client code is quite good, and we have with great success used npm to manage our client side dependencies and installed dozens of modules from npm (hundreds if you count transitive dependencies). You may not enjoy that way of working, possibly because it does not use a format you created, but many others do, and trying to squash what is unequivocally a good change seems mean-spirited and counterproductive.

Contributor

isaacs commented Jan 28, 2013

Global re-exposed on a3601e361418e3a7e91d6ad03be9b87a4870ad45.

Any other issues to address?

"npm in jquery" points to not putting front end code in npm.
...
Node's module system is not a good fit for direct use in the browser, and there will always need to be a bundled format for use in the browser.

Ok, that's nice. Take it up with the people using Browserify. Unless your bottom line is that jQuery should not support the Node module environment at all, and this pull request should be rejected, it is out of scope for this discussion.

Whatever the case, people ARE using jquery in Node now on the server, and they ARE using jquery in Browserify, and leaking a global is bad behavior in a Node module environment. This pull request is only about correcting that bad behavior, and getting us closer to the jQuery team owning jQuery in npm.

@dcherman dcherman commented on an outdated diff Jan 28, 2013

src/exports.js
@@ -15,4 +12,14 @@ window.jQuery = window.$ = jQuery;
// noConflict to hide this version of jQuery, it will work.
if ( typeof define === "function" && define.amd && define.amd.jQuery ) {
define( "jquery", [], function () { return jQuery; } );
+ window.jQuery = window.$ = jQuery;
+} else if ( typeof module === "object" && typeof module.exports === "object" ) {
@dcherman

dcherman Jan 28, 2013

Contributor

If you check this condition first, then in the else you should be guaranteed to have to export the globals so you wouldn't need to duplicate that code.

Contributor

jrburke commented Jan 28, 2013

@domenic: sorry, I mixed some things in my message. If you want to use npm to move code around, that is fine. That issue is separate from the main one here, I apologize for getting distracted.

For whatever you use to deploy the code to the browser, the bundling format should probably just use AMD though.

@isaacs: yes, my comments were suggesting that if this pull request to support the node environment was so that it would work in a node environment, that is out of scope for jQuery because jQuery cannot be used in node as-is.

document and window are not in node. If they do show up there, it is because of a node module providing globals for them, and if globals are in play, this is likely just a browser simulation environment that does not need jQuery as a node module. In addition, expecting all browser code, like jQuery plugins, to fit into the "no globals" pattern has not worked. We tried that in AMD, and got real data that it causes more support headaches for the library (what happened with underscore/backbone).

So, if this pull request is for:

  • running jQuery in node: that is likely the browser simulation environment, and jQuery does not need this change. There are much larger issues with running jQuery in node, and other code run will likely want jQuery as a global. Maybe there are use cases I don't know about though that do not fit into this description.
  • supporting node module systems in the browser: those systems already need a bundling format. AMD has the mindshare there, and jQuery already supports it. Trying to support lots of different module formats in the browser seems counterproductive at this point, unless it is for ES modules.
Contributor

isaacs commented Jan 28, 2013

running jQuery in node: that is likely the browser simulation environment, and jQuery does not need this change. There are much larger issues with running jQuery in node, and other code run will likely want jQuery as a global. Maybe there are use cases I don't know about though that do not fit into this description.

I have received complaints that the jquery module on npm includes a browser simulation, and that it leaks globals, on github, twitter, and via email. My answer to these requests was traditionally, "Not my problem, take it up with jQuery/etc." but eventually I decided to get involved with this pull req and discussions with the jquery team and the maintainers of the existing npm modules named "jquery".

You are saying that they're going to need to use a global jQuery anyway, but they're saying otherwise, and I'm inclined to believe them rather than you regarding what they need.

supporting node module systems in the browser: those systems already need a bundling format. AMD has the mindshare there, and jQuery already supports it. Trying to support lots of different module formats in the browser seems counterproductive at this point, unless it is for ES modules.

I don't really know what "mindshare" actually means in this context, and I'm confused as to why AMD's possession of this "mindshare" would mean that real world node and Browserify users who are complaining about this should be forced to leak globals when their module system does not need this behavior.

Just to be clear, it sounds like your position on this entire pull request is that you are against it, in principle, because it allows people to do things that you think are a bad idea (to wit: a. putting jquery on npm, b. using jquery in node, and c. bundling jquery with a loader other than AMD).

However, some other people are very much for this change (in principle, that is; nits regarding the implementation notwithstanding), and I fail to see how your concerns are relevant to them, or how this minimal patch will affect you, or any users of your libraries. If you can show how it actually negatively impacts AMD, or AMD users, or any use case you actually have, then great, let's hear it. But if your only complaint is that it'll help people who are doing a thing you disagree with, then that's really not any of your business, afaict.

@dmethvin dmethvin and 1 other commented on an outdated diff Jan 28, 2013

src/sizzle
@@ -1 +1 @@
-Subproject commit c0d9badeb5dac649d0ba7824efaebbe4155369b5
+Subproject commit 19c7b3440385c9f628a7bc1c5769f6946fcc6887
@dmethvin

dmethvin Jan 28, 2013

Owner

This shouldn't be here, there were no Sizzle changes.

@rwaldron

rwaldron Jan 28, 2013

Member

I was able to pull this off without affecting the rest of the patch. I can land it if you're good to go

*by "able", I just used Tower's "discard local changes" ;)

Contributor

jrburke commented Jan 28, 2013

@isaacs: great, thanks for providing more info. Maybe you and the jQuery team already discussed those use cases, so I'm sorry if this is a rehash. Nice to have them written down future reference though.

Hiding the global is likely to generate support requests to jQuery when the user in the browser global context you describe then tries to load a jquery plugin that wants jquery as a global. I'm not suggesting anything about AMD in this case, only that it is a module format that has tried loading browser code and we found data to that effect. It is a real data point from something that tried something similar. @domenic shared this particular concern too, and he seems to be a user that would benefit from this change.

While I can appreciate you get a lot of pings from node programmers complaining about what may seem a simple problem, it does not mean those complaints correctly identify the underlying problem. It may really be about a larger issue with simulating browsers in node, the expectation to being able to use just any browser code in node and/or the difficulties of not having a universal module system in JS. I still feel the use case is a bit blurry, but you may have already worked that out with the jQuery folks.

Just to be clear, it sounds like your position on this entire pull request is that you are against it, in principle, because it allows people to do things that you think are a bad idea (to wit: a. putting jquery on npm, b. using jquery in node, and c. bundling jquery with a loader other than AMD).

a. no, I already said the npm issue is not part of this issue and apologized for distraction. I should not have focused on that part of your previous comment.

b. if it creates support requests for jQuery because the use case and implementation is flawed, then yes. At the very least removing the global does seem flawed based on previous data.

c. similarly, this is a support issue. Lots of people have invented lots of ways to bundle client code into modules. jQuery cannot expect to support them all. Perhaps the jQuery people feel this change meets their minimum bar, which is fine. However it also seems like a reasonable answer to just suggest claim AMD for client bundling since it has already been tested and known to work, and does not add more to their support cost.

So, while you may have read my feedback as some "only do things my way" screed, my concerns were more about not having enough context about what this will really solve, you understanding of the second level effects, and the support cost it places on jQuery, particularly if it goes in without the global export.

Hopefully that summarizes my concerns, I leave it to the jQuery folks to sort out, unless there are questions directed to me.

Member

rwaldron commented Jan 28, 2013

@dmethvin I pulled this and seems to have all its ducks in a row—I had to discard the changes to Sizzle submodule commit pointer, but that's no big deal. Leaving this for you to check before landing.

@dmethvin dmethvin commented on the diff Jan 28, 2013

src/outro.js
@@ -1,2 +1,2 @@
-})( window );
+})( this );
@dmethvin

dmethvin Jan 28, 2013

Owner

If "use strict" is in effect, this isn't set to the global object is it? Also, window and the global object aren't always identical, I forget the specifics but IE 6/7/8 may have issues.

@domenic

domenic Jan 28, 2013

That would only be a concern if jQuery source was naively concattenated with other files that included bare "use strict"; not sure how much of a concern that is. If so, then I believe (false || eval)("this") will do the trick.

@dmethvin

dmethvin Jan 28, 2013

Owner

If someone were using this under node, would they want what jQuery considers to be the window object to be the global object, or would it be better for them to create a global window object and pass that in with whatever methods were needed? It seems like the former requires more pollution of the global namespace.

@domenic

domenic Jan 28, 2013

That's a good point. In my experience using jQuery in Node directly (as opposed to in browserify), I always load it in a sandbox which has the global equal to a jsdom window. So it doesn't make any difference in that case. And it doesn't make any difference in the browserify case either, where they're identical. /shrug

@isaacs

isaacs Jan 29, 2013

Contributor

In Node, this is the exports object (for reasons of historical weirdness). But if we're not setting a global reference, then it doesn't matter.

The surest "global getter" that I've ever seen is (function () { return this; }).call(null). You could do something like (function () { return this; }).call(null).window so that it's empty if there's no global window defined, or the global window in any web browser.

@isaacs

isaacs Jan 29, 2013

Contributor

Oh, I guess that doesn't work on "use strict" either.

Well, it can be just Yet Another case where a top-level "use strict" causes everything to break.

@rwaldron

rwaldron Jan 29, 2013

Member

Top level strict does nothing to top level "this"

"use strict"; (function(o){ "use strict";  return [this, o]; })(this);
> [undefined, window ] // the browser case
@isaacs

isaacs Jan 29, 2013

Contributor

Oh, nice. I guess I don't run my JavaScript at the top level much these days. Tend to forget the oddities :)

@rwaldron

rwaldron Jan 29, 2013

Member

Show off ;)

@Krinkle

Krinkle Jan 30, 2013

Member

Can we rely on this being in the global scope? What if a site concatenates this and other modules in one request and wraps it in a loader closure?

@rwaldron

rwaldron Jan 30, 2013

Member

Definite possibility

@isaacs

isaacs Jan 30, 2013

Contributor

@Krinkle Would you prefer doing something like typeof window === 'undefined' ? this : window?

@Krinkle

Krinkle Jan 30, 2013

Member

@isaacs Hm.. maybe, but why can't it be just window. We only use it in a browser environment, right? In Node for example we set exports, or are we (ab)using the historical this being the exports object in Node?

@isaacs

isaacs Jan 30, 2013

Contributor

@Krinkle Well, no, that's the point. When run in Node, jquery requires a window to already be defined, or it throws an undefined reference at this point. It looks like you really do want it to be the global though, not some random object. (There's window.String stuff, it looks like.)

domenic commented Feb 20, 2013

What's left to get this landed? If you want a foolproof global, just do the usual ugly Function("return this")(). Anything else?

Member

rwaldron commented Feb 20, 2013

Aside from any concerns @dmethvin may have, it needs to be rebased.

Owner

dmethvin commented Feb 20, 2013

No remaining concerns here, and it would be great to land this before the next beta. Last call..

Member

rwaldron commented Feb 20, 2013

Great! @isaacs can you rebase? Thanks!

Support node-like module loaders
Also, do not pollute the global namespace in Node module loaders.
Contributor

isaacs commented Feb 20, 2013

Rebased onto upstream master.

@rwaldron rwaldron closed this in a128355 Feb 21, 2013

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