Skip to content

Fix UMD bundles, making safe to use as required modules #7840

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zpao merged 2 commits into from
Oct 3, 2016
Merged

Fix UMD bundles, making safe to use as required modules #7840

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac356bf
Attempt to fix UMD bundles
zpao Sep 30, 2016
cbe6e61
Use more standard wrapper for consistent code
zpao Oct 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions grunt/config/browserify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,52 @@ function simpleBannerify(src) {
);
}

// What is happening here???
// I'm glad you asked. It became really to make our bundle splitting work.
Copy link
Copy Markdown
Contributor

@aweary aweary Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It became really what?! 😄

Copy link
Copy Markdown
Member Author

@zpao zpao Oct 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too late! I pressed merge.

It became really "hard". Or maybe I wrote "annoying"? I'm going to leave the mystery in there for now :)

// Everything is fine in node and when bundling with those packages, but when
// using our pre-packaged files, the splitting didn't work. Specifically due to
// the UMD wrappers defining their own require and creating their own encapsulated
// "registry" scope, we couldn't require across the boundaries. Webpack tries to
// be smart and looks for top-level requires (even when aliasing to a bundle),
// but since we didn't have those, we couldn't require 'react' from 'react-dom'.
// But we are already shimming in some modules that look for a global React
// variable. So we write a wrapper around the UMD bundle that browserify creates,
// and define a React variable that will require across Webpack-boundaries or fall
// back to the global, just like it would previously.
function wrapperify(src) {
return `
;(function(f) {
// CommonJS
if (typeof exports === "object" && typeof module !== "undefined") {
f(require('react'));

// RequireJS
} else if (typeof define === "function" && define.amd) {
require(['react'], f);
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be define, not require.

Copy link
Copy Markdown
Member Author

@zpao zpao Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what I have is right. define is used to define the exports, which isn't what we get back from calling f. require just makes sure the module is available and runs code without exports. Same reason we don't do module.exports = f(require('react') for the CommonJS case. It would be define in a UMD wrapper and that is what browserify will build and that we call inside f.

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked in the docs and you're right. 😅


// <script>
} else {
var g;
if (typeof window !== "undefined") {
g = window;
} else if (typeof global !== "undefined") {
g = global;
} else if (typeof self !== "undefined") {
g = self;
} else {
// works providing we're not in "use strict";
// needed for Java 8 Nashorn
// see https://github.com/facebook/react/issues/3037
g = this;
}
f(g.React)
}
})(function(React) {
${src}
});
`;
}

// Our basic config which we'll add to to make our other builds
var basic = {
entries: [
Expand Down Expand Up @@ -137,7 +183,7 @@ var dom = {
transforms: [shimSharedModules],
globalTransforms: [envifyDev],
plugins: [collapser],
after: [derequire, simpleBannerify],
after: [derequire, wrapperify, simpleBannerify],
};

var domMin = {
Expand All @@ -155,7 +201,7 @@ var domMin = {
// No need to derequire because the minifier will mangle
// the "require" calls.

after: [minify, bannerify],
after: [wrapperify, minify, bannerify],
};

var domServer = {
Expand All @@ -169,7 +215,7 @@ var domServer = {
transforms: [shimSharedModules],
globalTransforms: [envifyDev],
plugins: [collapser],
after: [derequire, simpleBannerify],
after: [derequire, wrapperify, simpleBannerify],
};

var domServerMin = {
Expand All @@ -187,7 +233,7 @@ var domServerMin = {
// No need to derequire because the minifier will mangle
// the "require" calls.

after: [minify, bannerify],
after: [wrapperify, minify, bannerify],
};

var domFiber = {
Expand All @@ -201,7 +247,7 @@ var domFiber = {
transforms: [shimSharedModules],
globalTransforms: [envifyDev],
plugins: [collapser],
after: [derequire, simpleBannerify],
after: [derequire, wrapperify, simpleBannerify],
};

var domFiberMin = {
Expand All @@ -219,7 +265,7 @@ var domFiberMin = {
// No need to derequire because the minifier will mangle
// the "require" calls.

after: [minify, bannerify],
after: [wrapperify, minify, bannerify],
};

module.exports = {
Expand Down