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 AMD and Brunch issues #8686

Merged
merged 5 commits into from
Jan 5, 2017
Merged

Fix AMD and Brunch issues #8686

merged 5 commits into from
Jan 5, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jan 4, 2017

This fixes #8392 and adds manual build fixtures so we can more easily check regressions like this.

We used to read ReactDOM as a global inside ReactAddonsDOMDependenciesUMDShim.
This didn't work in AMD environments such as RequireJS and SystemJS.

Instead, I changed it so that ReactDOM gets injected into ReactWithAddons by ReactDOM itself.
This way we don't have to try to require it (which wouldn't work because AMD doesn't handle circular dependencies well).

This means you have to load ReactDOM first before using ReactDOM-dependent addons, but this was already the case before.

This commit makes all build fixtures pass.
Build size changes appear insignificant.

@@ -0,0 +1,10 @@
{
"name": "systemjs-builder-test",

Choose a reason for hiding this comment

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

I guess you meant to name this "rjs-test". Apart from that, there's not much I can contribute here I'm afraid. :-(

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 5, 2017

For SystemJS, it seems like we can force it to prefer CommonJS (which solves the issue) by mangling define call. However this breaks r.js optimizer. I wonder if anybody still uses r.js with React these days.

var toReplace =
`function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define([],f)}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.${this.data.standalone} = f()}}`;
if (src.indexOf(toReplace) === -1) {
throw new Error('wrapperify failed to find code to replace');
Copy link

Choose a reason for hiding this comment

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

may want to refer to the wrapperify instance in this error as wrapperifyAddons

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, doesn't really matter for now, this is all too confusing and broken to get it in.
I'm thinking of alternate approaches now.

@gaearon gaearon force-pushed the fix-amd branch 4 times, most recently from bff912e to baac473 Compare January 5, 2017 18:10
@gaearon gaearon changed the title [WIP] Wrap ReactWithAddons into a custom wrapper Fix ReactWithAddons build in AMD environment Jan 5, 2017
nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children,
ReactAddonsDOMDependencies.getReactInstanceMap().get(this)._debugID
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I opted to just remove this dependency. Its only use is to add the stack trace in this particular case. I think we can live without it like it was in 0.14 (and it wouldn't be able to do this in userland anyway).

var ReactDOM = require('ReactDOM');

var ReactDOMUMDEntry = Object.assign({
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
ReactInstanceMap: require('ReactInstanceMap'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was only used for that stack trace so we can remove it too now.

// Inject ReactDOM into React for the addons UMD build that depends on ReactDOM (TransitionGroup).
// We can remove this after we deprecate and remove the addons UMD build.
if (React.addons) {
React.__SECRET_INJECTED_REACT_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = ReactDOMUMDEntry;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where we inject ourselves into React now.

return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
var ReactWithAddonsUMDEntry = require('ReactWithAddonsUMDEntry');
// This is injected by the ReactDOM UMD build:
return ReactWithAddonsUMDEntry.__SECRET_INJECTED_REACT_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The worst case (it doesn't exist) is the same worst case we already had before when ReactDOM wasn't available yet, but at least this works with AMD. I don't think it's realistic that people will get here though, unless they have mismatching React versions (which breaks in other subtle ways anyway).

@@ -11,24 +11,24 @@

'use strict';

var React = require('React');
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is safe... doesn't it result in React getting packaged into the ReactDOM bundle? I would expect build/react-dom.js just exploded in size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so—don't we requite React all over the place in ReactDOM? For example ReactCompositeComponent (now part of ReactDOM) uses React.isValidElement. I think @sebmarkbage made it work during the package split.. somehow. Maybe with this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(And I'm not seeing any noticeable changes in size, it's all within 100 bytes.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah yea, that get's rewritten to the ReactUMDShim. Sorry, just experiencing some light PTSD thinking about making AMD again. Carry on then :)

We used to read ReactDOM as a global inside ReactAddonsDOMDependenciesUMDShim.
This didn't work in AMD environments such as RequireJS and SystemJS.

Instead, I changed it so that ReactDOM gets injected into ReactWithAddons by ReactDOM itself.
This way we don't have to try to require it (which wouldn't work because AMD doesn't handle circular dependencies well).

This means you have to load ReactDOM first before using ReactDOM-dependent addons, but this was already the case before.

This commit makes all build fixtures pass.
@gaearon
Copy link
Collaborator Author

gaearon commented Jan 5, 2017

@zpao @sebmarkbage @spicyj Should be ready for review, who wants to do it?

return ReactDOM;
};
function getReactDOM() {
var ReactWithAddonsUMDEntry = require('ReactWithAddonsUMDEntry');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be safe since we only use ReactAddonsDOMDependenciesUMDShim in the addons build. Again, build sizes looked normal.

Copy link
Member

@zpao zpao left a comment

Choose a reason for hiding this comment

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

Otherwise, seems like this should be fine. I'll accept but up to you if you think anybody else who's been paying closer attention recently needs to take a look.

return ReactDOM;
};
function getReactDOM() {
var ReactWithAddonsUMDEntry = require('ReactWithAddonsUMDEntry');
Copy link
Member

Choose a reason for hiding this comment

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

Probably not a huge deal in the grand scheme, but could potentially memoize this so we don't have to enter require on every call.

@gaearon gaearon changed the title Fix ReactWithAddons build in AMD environment Fix ReactWithAddons build in AMD environments and Brunch Jan 5, 2017
@gaearon gaearon changed the title Fix ReactWithAddons build in AMD environments and Brunch Fix ReactWithAddons build in AMD environments Jan 5, 2017
@gaearon gaearon changed the title Fix ReactWithAddons build in AMD environments Fix ReactWithAddons build in AMD environment Jan 5, 2017
@gaearon gaearon changed the title Fix ReactWithAddons build in AMD environment Fix AMD and Brunch issues Jan 5, 2017
@gaearon gaearon added this to the 15-next milestone Jan 5, 2017
@gaearon
Copy link
Collaborator Author

gaearon commented Jan 5, 2017

@ghost
Copy link

ghost commented Jan 5, 2017

@gaearon awesome job, you just made all the brunch(er) really happy ;)

@gaearon gaearon modified the milestones: 15-hipri, 15-lopri, 15.4.2 Jan 6, 2017
gaearon added a commit that referenced this pull request Jan 6, 2017
* Add manual build fixtures

* Inject ReactDOM into ReactWithAddons from ReactWithAddons

We used to read ReactDOM as a global inside ReactAddonsDOMDependenciesUMDShim.
This didn't work in AMD environments such as RequireJS and SystemJS.

Instead, I changed it so that ReactDOM gets injected into ReactWithAddons by ReactDOM itself.
This way we don't have to try to require it (which wouldn't work because AMD doesn't handle circular dependencies well).

This means you have to load ReactDOM first before using ReactDOM-dependent addons, but this was already the case before.

This commit makes all build fixtures pass.

* Memoize ReactDOM to avoid going into require on every access

* Add Brunch fixture

* Inline requires to work around Brunch bug

See #8556 and brunch/brunch#1591 (comment) for context.
This appears to be a Brunch bug but we can keep a temporary fix until the next major.

(cherry picked from commit ca2c71c)
@Restuta
Copy link

Restuta commented Jan 8, 2017

After upgrading to 15.4.2 I started getting:

WARNING in ./~/react-dom/dist/react-dom.js
Critical dependencies:
35:192-199 This seems to be a pre-built javascript file. Though this is possible, it's not recommended. Try to require the original source to get better results.
 @ ./~/react-dom/dist/react-dom.js 35:192-199

As part of my webpack build, I include react-dom as a noParse option in webpack and set alias pointing to react-dom/dist/react-dom.js. Any quick thoughts on why I am seeing this? @gaearon

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 9, 2017

Please bring it up with Webpack, it is their decision to emit a message in this case. There is nothing wrong with your build. (But note that in production you want react-dom.min.js.)

@VinSpee
Copy link

VinSpee commented Jan 26, 2017

@gaearon

I wonder if anybody still uses r.js with React these days.

FYI we're still using React w/ RequireJS and the r.js optimizer on TED.com -We're planning migration to webpack eventually but not for a while.

@gaearon
Copy link
Collaborator Author

gaearon commented Jan 26, 2017

👍
Glad we fixed it then.

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.

Cannot use addons.Perf or addons.ReactTransitionGroup in AMD environment with 15.4.1
7 participants