-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Conversation
5ae3cd2
to
ff9fd60
Compare
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 |
ff9fd60
to
cbe6e61
Compare
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. |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It became really what?! 😄
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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. 😅
(cherry picked from commit 92c84a6)
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:
cc @facebook/react-core
Fixes #7482
Test Plan:
react
andreact-dom
.