Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#10545 closed bug (wontfix)

Don't expose jQuery to `window` with AMD

Reported by: rpflorence Owned by:
Priority: high Milestone: None
Component: core Version: 1.7b2
Keywords: Cc:
Blocked by: Blocking:

Description

Pull request: https://github.com/jquery/jquery/pull/557

One of the great features of AMD is not touching the global namespace outside of require and define.

I don't want my team and I to use jQuery w/o deliberating requiring it into a module, additionally the RequireJS optimizer will know to include it.

For people requiring from a CDN, they just configure the paths, no need for a global jQuery object in an AMD environment:

var require = {

paths: {

"jquery": "http://code.jquery.com/jquery-1.7b2.js"

}

};

Change History (6)

comment:1 Changed 7 years ago by addyosmani

Component: unfiledcore
Priority: undecidedhigh
Status: newopen

comment:2 Changed 7 years ago by jrburke

Posted this in the pull request, did not see the ticket until after posting. Adding the comment here:


I appreciate wanting to do this, but I'm not sure it is safe to do so given how much jQuery is used. One of the reasons the original AMD patch was pulled from 1.6 was concerns about how multiple versions of jQuery would interact with AMD, since some pages can inadvertently load more than one version of jQuery.

The define.amd.jQuery was added so that an AMD loader could broadcast that it knew there could be a few jQuerys loaded on a page and had mitigations so that the developer who was using an AMD loader could receive the correct one. In requirejs, there is multversion support and also a way in the config to specify only accepting a specific version of jquery in the define call.

My concern with this patch is that a third party lib that uses jQuery is loaded on a page with an AMD loader. This patch would mean the AMD loader would get that version of jQuery but the third party lib would not see a global jQuery object, and errors would occur. What is worse, the AMD loader may just outright reject that jQuery registration call if it is looking for a different jQuery version.

If jQuery were not as popular as it is now, or started off always with this logic, it would not be that much of a concern. In fact if it was any other JS library out there, the approach in this patch is the right one. However, for jQuery, I believe it will lead to errors and elevated support requests.

I do not have a complete understanding of the magnitude of this hazard, the jQuery core folks can answer that better. But this concern is what lead me doing the AMD registration as it is today.

For AMD folks that do not want to export a global, they can do an optimized build and call jQuery.noConflict() to get this effect (with the build there should not be a window when the AMD-desired jQuery interferes with another one in the page). Not the ideal, but sometimes these kinds of things are needed to upgrade the web.

It would be good to get feedback on what people think is the relative threat of problems as outlined above, but I think given jQuery's special status doing the dual registration is the less disruptive route.

comment:3 Changed 7 years ago by jrburke

Initial AMD registration and discussion in this ticket:

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

comment:4 Changed 7 years ago by timmywil

Resolution: wontfix
Status: openclosed

We agree that it would be ideal, but probably not realistic.

comment:5 Changed 6 years ago by Nathan Friedly <nathan@…>

I'd like to see this done, even if it's saved for a fairly major release (something after 1.9/2.0 since those two are supposed to be API-compatible) - this specific bug just caused a production outage in one of our customer's websites because my company switched to AMD (requirejs) and even though we called jQuery.noConflict(true) as soon as our code runs, one of their plugins got attached to our copy of jQuery instead of theirs in the few milliseconds between when our copy of jQuery loaded and when our requirejs callback ran.

comment:6 Changed 6 years ago by dmethvin

See the discussion in the pull request referenced at the top of the ticket above.

Note: See TracTickets for help on using tickets.