http://bugs.jquery.com/ticket/13119

Theelse statements should start on the same line as the preceedingIfStatement's closing curly, with the comments immediately inside the block. Thanks :)

Style fixups squashed into 90cc01f5bf3d360783f88daf56000e94b0f6eae2.

+1

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

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.

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.

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

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 definitelynot create a global. That's considered bad behavior.

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 :-/.

Really? In the browser, there's going to be amodule object with anexports member?

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

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.

@jrburke People are using jquery in Node today. But it's a huge confusing pita, becausenpm 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-liketransport mechanism, the API provided to theuser clearly does not depend on leaking globals. What the user will expect is thatvar $ = 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.

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

With that in mind, leaking globalsis 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 doesvar $ = require("jquery") load the jquery function, but also that<script src="jquery.color.js"></script> works on the same page.

@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.

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.

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.

@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.

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.

@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.

@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.

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

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

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

Great!@isaacs can you rebase? Thanks!

Rebased onto upstream master.