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

Bug fix: SSR setState in diff components don't mix #12323

Merged
merged 1 commit into from
Mar 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 34 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,38 @@ describe('ReactDOMServerLifecycles', () => {
ReactDOMServer.renderToString(<Component />);
expect(log).toEqual(['componentWillMount', 'UNSAFE_componentWillMount']);
});

it('tracks state updates across components', () => {
class Outer extends React.Component {
UNSAFE_componentWillMount() {
this.setState({x: 1});
}
render() {
return <Inner updateParent={this.updateParent}>{this.state.x}</Inner>;
}
updateParent = () => {
this.setState({x: 3});
};
}
class Inner extends React.Component {
UNSAFE_componentWillMount() {
this.setState({x: 2});
this.props.updateParent();
}
render() {
return <div>{this.props.children + '-' + this.state.x}</div>;
}
}
expect(() => {
// Shouldn't be 1-3.
expect(ReactDOMServer.renderToStaticMarkup(<Outer />)).toBe(
'<div>1-2</div>',
);
}).toWarnDev(
'Warning: setState(...): Can only update a mounting component. This ' +
'usually means you called setState() outside componentWillMount() on ' +
'the server. This is a no-op.\n\n' +
'Please check the code for the Outer component.',
);
});
});
56 changes: 24 additions & 32 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,36 +381,26 @@ function resolve(
child: mixed,
context: Object,
|} {
let element: ReactElement;

let Component;
let publicContext;
let inst, queue, replace;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

queue and replace were problematic; I moved the rest too for clarity.

let updater;

let initialState;
let oldQueue, oldReplace;
let nextState, dontMutate;
let partial, partialState;

let childContext;
let childContextTypes, contextKey;

while (React.isValidElement(child)) {
// Safe because we just checked it's an element.
element = ((child: any): ReactElement);
let element: ReactElement = (child: any);
let Component = element.type;
if (__DEV__) {
pushElementToDebugStack(element);
}
Component = element.type;
if (typeof Component !== 'function') {
break;
}
publicContext = processContext(Component, context);
processChild(element, Component);
}

// Extra closure so queue and replace can be captured properly
function processChild(element, Component) {
let publicContext = processContext(Component, context);

queue = [];
replace = false;
updater = {
let queue = [];
let replace = false;
let updater = {
isMounted: function(publicInstance) {
return false;
},
Expand All @@ -433,6 +423,7 @@ function resolve(
},
};

let inst;
if (shouldConstruct(Component)) {
inst = new Component(element.props, publicContext, updater);

Expand All @@ -453,7 +444,7 @@ function resolve(
}
}

partialState = Component.getDerivedStateFromProps.call(
let partialState = Component.getDerivedStateFromProps.call(
null,
element.props,
inst.state,
Expand Down Expand Up @@ -502,15 +493,15 @@ function resolve(
if (inst == null || inst.render == null) {
child = inst;
validateRenderResult(child, Component);
continue;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these equivalent? What was happening before, when the loop continued to another iteration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isValidElement would return false and the loop would break. returning from the function here is equivalent to continue before, since there's nothing in the loop body after this function call.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍, thanks!

}
}

inst.props = element.props;
inst.context = publicContext;
inst.updater = updater;

initialState = inst.state;
let initialState = inst.state;
if (initialState === undefined) {
inst.state = initialState = null;
}
Expand Down Expand Up @@ -558,19 +549,19 @@ function resolve(
inst.UNSAFE_componentWillMount();
}
if (queue.length) {
oldQueue = queue;
oldReplace = replace;
let oldQueue = queue;
let oldReplace = replace;
queue = null;
replace = false;

if (oldReplace && oldQueue.length === 1) {
inst.state = oldQueue[0];
} else {
nextState = oldReplace ? oldQueue[0] : inst.state;
dontMutate = true;
let nextState = oldReplace ? oldQueue[0] : inst.state;
let dontMutate = true;
for (let i = oldReplace ? 1 : 0; i < oldQueue.length; i++) {
partial = oldQueue[i];
partialState =
let partial = oldQueue[i];
let partialState =
typeof partial === 'function'
? partial.call(inst, nextState, element.props, publicContext)
: partial;
Expand Down Expand Up @@ -600,11 +591,12 @@ function resolve(
}
validateRenderResult(child, Component);

let childContext;
if (typeof inst.getChildContext === 'function') {
childContextTypes = Component.childContextTypes;
let childContextTypes = Component.childContextTypes;
if (typeof childContextTypes === 'object') {
childContext = inst.getChildContext();
for (contextKey in childContext) {
for (let contextKey in childContext) {
invariant(
contextKey in childContextTypes,
'%s.getChildContext(): key "%s" is not defined in childContextTypes.',
Expand Down