Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fixes #14549. Execute the factory immediately when CommonJS is used i…
…n the browser.
  • Loading branch information
timmywil committed Nov 15, 2013
1 parent 6fde975 commit 8f7db68
Showing 1 changed file with 14 additions and 10 deletions.
24 changes: 14 additions & 10 deletions src/intro.js
Expand Up @@ -15,17 +15,21 @@
(function( window, factory ) {

if ( typeof module === "object" && typeof module.exports === "object" ) {
// Expose a jQuery-making factory as module.exports in loaders that implement the Node
// module pattern (including browserify).
// This accentuates the need for a real window in the environment
// For CommonJS and CommonJS-like environments where a proper window is present,
// execute the factory and get jQuery
// For environments that do not inherently posses a window with a document
// (such as Node.js), expose a jQuery-making factory as module.exports
// This accentuates the need for the creation of a real window
// e.g. var jQuery = require("jquery")(window);
module.exports = function( w ) {
w = w || window;
if ( !w.document ) {
throw new Error("jQuery requires a window with a document");
}
return factory( w );
};
// See ticket #14549 for more info
module.exports = window.document ?

This comment has been minimized.

Copy link
@domenic

domenic Nov 15, 2013

Don't you mean typeof window !== "undefined" && window.document?

This comment has been minimized.

Copy link
@timmywil

timmywil Nov 15, 2013

Author Member

It refers to the global object, which is always there. Whether it has a document is what needs to be determined.

This comment has been minimized.

Copy link
@domenic

domenic Nov 15, 2013

Got it, cool :)

This comment has been minimized.

Copy link
@mgol

mgol Nov 16, 2013

Member

I'd prefer to rename window to global in these first 32 lines of code. It seems much clearer for me to read if (global.document) and think "global has a document so this global is a window" than to read about window that isn't necessarily a window.

Such a change would only affect this very beginning of jQuery code anyway. What do you think?

This comment has been minimized.

Copy link
@timmywil

timmywil via email Nov 16, 2013

Author Member

This comment has been minimized.

Copy link
@mgol

mgol Nov 17, 2013

Member

Landed at dc649a3

This comment has been minimized.

Copy link
@ForbesLindesay

ForbesLindesay Jan 8, 2014

Contributor

This doesn't work in browserify since this is not actually a global, it's the module.exports value, so the test always fails even though we're in a browser.

This comment has been minimized.

Copy link
@mgol

mgol Jan 8, 2014

Member

@ForbesLindesay So there's no other way than to explicitely check if the global variable is declared?

This comment has been minimized.

Copy link
@timmywil

timmywil Jan 8, 2014

Author Member

Hmm, that does change things. I'm a little surprised though. Every browser library has thus far been able to assume it runs in the browser's context. Doesn't the context change break other libraries? Also, one of the proposed features of browserify is the ability to use Node modules in the browser and I imagine that there are Node modules that assume they run in Node's context. Why complicate things?

This comment has been minimized.

Copy link
@dmethvin

dmethvin Jan 8, 2014

Member

Not sure myself, but I'm inclined to punt on Browserify support for 1.11/2.1 unless we can resolve this today. Shouldn't be making changes like this at an RC.

This comment has been minimized.

Copy link
@timmywil

timmywil Jan 8, 2014

Author Member

Well, I can fix it, but I'm rolling my eyes.

This comment has been minimized.

Copy link
@dmethvin

dmethvin Jan 8, 2014

Member

Best I can do is 🎢 👀 . It does seem like Browserify would want to use Node conventions for that reason. @ForbesLindesay is that likely to change?

This comment has been minimized.

Copy link
@mgol

mgol Jan 8, 2014

Member

I'm happy we're doing an RC. But if after releasing RC there is still problem with this intro, I'd go for reversing the patch and giving up on browserify support at least for 1.11/2.1. I agree it's too dangerous to experiment like that.

This comment has been minimized.

Copy link
@mgol

mgol Jan 8, 2014

Member

@timmywil @dmethvin I put some comments in the PR by @ForbesLindesay: #1476.

This comment has been minimized.

Copy link
@ForbesLindesay

ForbesLindesay Jan 9, 2014

Contributor

Please don't revert what you have. What you currently have works, it just has the frustrating restriction that you have to call requrie('jquery')(window) to get a reference to jQuery and that you get a new jQuery every time you call that function.

This comment has been minimized.

Copy link
@terinjokes

terinjokes Jan 10, 2014

If the first check fails, can we check against self and self.document? That would satisfy browserify.

This comment has been minimized.

Copy link
@dmethvin

dmethvin Jan 10, 2014

Member

At this point I am concerned that nothing will satisfy browserify but I landed 6de1d97 for now. There's always 1.11.1 🔹

This comment has been minimized.

Copy link
@ForbesLindesay

ForbesLindesay Jan 10, 2014

Contributor

I'm pretty sure browserify is satisfied now.

factory( window ) :
function( w ) {
if ( !w.document ) {
throw new Error("jQuery requires a window with a document");
}
return factory( w );
};
} else {
factory( window );
}
Expand Down

0 comments on commit 8f7db68

Please sign in to comment.