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.

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

Jump to bottom

Fix AMD and Brunch issues #8686

Merged
merged 5 commits into from
Jan 5, 2017
Merged

Fix AMD and Brunch issues #8686

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cebff96
Add manual build fixtures
gaearon Jan 5, 2017
db5b7ab
Inject ReactDOM into ReactWithAddons from ReactWithAddons
gaearon Jan 5, 2017
3302174
Memoize ReactDOM to avoid going into require on every access
gaearon Jan 5, 2017
fe8d64e
Add Brunch fixture
gaearon Jan 5, 2017
593c70e
Inline requires to work around Brunch bug
gaearon Jan 5, 2017
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
3 changes: 0 additions & 3 deletions scripts/fiber/tests-passing-except-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
src/addons/transitions/__tests__/ReactTransitionGroup-test.js
* should warn for duplicated keys with component stack info

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
* should not warn when server-side rendering `onScroll`
* should warn about incorrect casing on properties (ssr)
Expand Down
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js
* should handle enter/leave/enter/leave correctly
* should handle enter/leave/enter correctly
* should handle entering/leaving several elements at once
* should warn for duplicated keys

src/isomorphic/children/__tests__/ReactChildren-test.js
* should support identity for simple
Expand Down
5 changes: 0 additions & 5 deletions src/addons/ReactAddonsDOMDependencies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,11 @@
'use strict';

var ReactDOM = require('ReactDOM');
var ReactInstanceMap = require('ReactInstanceMap');

exports.getReactDOM = function() {
return ReactDOM;
};

exports.getReactInstanceMap = function() {
return ReactInstanceMap;
};

if (__DEV__) {
var ReactPerf = require('ReactPerf');
var ReactTestUtils = require('ReactTestUtils');
Expand Down
57 changes: 12 additions & 45 deletions src/addons/transitions/ReactTransitionGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'use strict';

var React = require('React');
var ReactAddonsDOMDependencies = require('ReactAddonsDOMDependencies');
var ReactTransitionChildMapping = require('ReactTransitionChildMapping');

var emptyFunction = require('emptyFunction');
Expand Down Expand Up @@ -56,17 +55,9 @@ class ReactTransitionGroup extends React.Component {
}

componentWillReceiveProps(nextProps) {
var nextChildMapping;
if (__DEV__) {
nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children,
ReactAddonsDOMDependencies.getReactInstanceMap().get(this)._debugID
);
} else {
nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children
);
}
var nextChildMapping = ReactTransitionChildMapping.getChildMapping(
nextProps.children
);
var prevChildMapping = this.state.children;

this.setState({
Expand Down Expand Up @@ -129,17 +120,9 @@ class ReactTransitionGroup extends React.Component {

delete this.currentlyTransitioningKeys[key];

var currentChildMapping;
if (__DEV__) {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children,
ReactAddonsDOMDependencies.getReactInstanceMap().get(this)._debugID
);
} else {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
}
var currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);

if (!currentChildMapping || !currentChildMapping.hasOwnProperty(key)) {
// This was removed before it had fully appeared. Remove it.
Expand Down Expand Up @@ -169,17 +152,9 @@ class ReactTransitionGroup extends React.Component {

delete this.currentlyTransitioningKeys[key];

var currentChildMapping;
if (__DEV__) {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children,
ReactAddonsDOMDependencies.getReactInstanceMap().get(this)._debugID
);
} else {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
}
var currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);

if (!currentChildMapping || !currentChildMapping.hasOwnProperty(key)) {
// This was removed before it had fully entered. Remove it.
Expand Down Expand Up @@ -210,17 +185,9 @@ class ReactTransitionGroup extends React.Component {

delete this.currentlyTransitioningKeys[key];

var currentChildMapping;
if (__DEV__) {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children,
ReactAddonsDOMDependencies.getReactInstanceMap().get(this)._debugID
);
} else {
currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);
}
var currentChildMapping = ReactTransitionChildMapping.getChildMapping(
this.props.children
);

if (currentChildMapping && currentChildMapping.hasOwnProperty(key)) {
// This entered again before it fully left. Add it again.
Expand Down
12 changes: 3 additions & 9 deletions src/addons/transitions/__tests__/ReactTransitionGroup-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ var ReactTransitionGroup;
describe('ReactTransitionGroup', () => {
var container;

function normalizeCodeLocInfo(str) {
return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

beforeEach(() => {
React = require('React');
ReactDOM = require('ReactDOM');
Expand Down Expand Up @@ -296,7 +292,7 @@ describe('ReactTransitionGroup', () => {
]);
});

it('should warn for duplicated keys with component stack info', () => {
it('should warn for duplicated keys', () => {
spyOn(console, 'error');

class Component extends React.Component {
Expand All @@ -315,13 +311,11 @@ describe('ReactTransitionGroup', () => {
'Child keys must be unique; when two children share a key, ' +
'only the first child will be used.'
);
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe(
expectDev(console.error.calls.argsFor(1)[0]).toBe(
'Warning: flattenChildren(...): ' +
'Encountered two children with the same key, `1`. ' +
'Child keys must be unique; when two children share a key, ' +
'only the first child will be used.\n' +
' in ReactTransitionGroup (at **)\n' +
' in Component (at **)'
'only the first child will be used.'
);
});
});
28 changes: 14 additions & 14 deletions src/umd/ReactDOMUMDEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :)

var ReactDOM = require('ReactDOM');

var ReactDOMUMDEntry = Object.assign({
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
ReactInstanceMap: require('ReactInstanceMap'),
},
}, ReactDOM);
var ReactDOMUMDEntry = ReactDOM;

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.

if (__DEV__) {
Object.assign(
ReactDOMUMDEntry.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED,
{
// ReactPerf and ReactTestUtils currently only work with the DOM renderer
// so we expose them from here, but only in DEV mode.
ReactPerf: require('ReactPerf'),
ReactTestUtils: require('ReactTestUtils'),
}
);
ReactDOMUMDEntry.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = {
// ReactPerf and ReactTestUtils currently only work with the DOM renderer
// so we expose them from here, but only in DEV mode.
ReactPerf: require('ReactPerf'),
ReactTestUtils: require('ReactTestUtils'),
};
}

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

}

module.exports = ReactDOMUMDEntry;
1 change: 1 addition & 0 deletions src/umd/ReactWithAddonsUMDEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var ReactWithAddons = require('ReactWithAddons');

// `version` will be added here by the React module.
var ReactWithAddonsUMDEntry = Object.assign({
__SECRET_INJECTED_REACT_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: null, // Will be injected by ReactDOM UMD build.
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
ReactCurrentOwner: require('ReactCurrentOwner'),
},
Expand Down
18 changes: 8 additions & 10 deletions src/umd/shims/ReactAddonsDOMDependenciesUMDShim.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,22 @@
* @providesModule ReactAddonsDOMDependenciesUMDShim
*/

/* globals ReactDOM */

'use strict';

exports.getReactDOM = function() {
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

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.

// 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).

}

exports.getReactInstanceMap = function() {
return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactInstanceMap;
};
exports.getReactDOM = getReactDOM;

if (__DEV__) {
exports.getReactPerf = function() {
return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactPerf;
};

exports.getReactTestUtils = function() {
return ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
return getReactDOM().__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactTestUtils;
};
}