Skip to content
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.

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

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

Merged
merged 2 commits into from
Oct 3, 2016

Conversation

zpao
Copy link
Member

@zpao zpao commented Sep 30, 2016

I think this fixes the problem. There's a giant comment in there explaining why and how.

I want to stress that this is a short-term solution. It works and the reasoning is sound but I'm not happy that we need this. It's probably time to start using webpack to bundle but that would have taken me much longer to go do.

Byte-wise this should be fine. We're just adding the code that you can see in there. Since we do this after the bundle process is done, that require('react') isn't analyzed.

Todo:

  • Look for any other expected globals in shims
  • Use this in other bundles
  • Look into the RequireJS situation. I honestly don't care much about that but we should probably make it work. It would require rejiggering the wrapper (since we couldn't execute the UMDed module immediately)

cc @facebook/react-core

Fixes #7482

Test Plan:

  • Made a hello world app with webpack and aliased react and react-dom.
  • Ran the same hello world using the dist files directly (so React & ReactDOM were globals).

@zpao
Copy link
Member Author

zpao commented Oct 3, 2016

The other case we have is ReactWithAddons, which has a shim that uses a global ReactDOM. However we've never supported using ReactWithAddons as a drop-in replacement in a require system. It should continue to work fine when using globals (eg script tags) just not replacing require('react/addons'). We'd get into a circular dep situation.

@zpao
Copy link
Member Author

zpao commented Oct 3, 2016

Ok, got this working and tested for ReactDOM/Server. I haven't tested the AMD stuff (meh) but I think it's right now. @gaearon gave me the thumbs up so I'm going to just merge and get 15.4 RC out.

@zpao zpao added this to the 15-next milestone Oct 3, 2016
@zpao zpao closed this Oct 3, 2016
@zpao zpao reopened this Oct 3, 2016
@zpao zpao merged commit 92c84a6 into facebook:master Oct 3, 2016
@@ -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
Contributor

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
Member Author

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 :)


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

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
Member Author

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
Collaborator

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

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016
zpao added a commit that referenced this pull request Oct 4, 2016
acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Master UMD builds don’t work as CommonJS using Webpack alias config
4 participants