Skip to content

bailoutOnLowPriority pushes context to mirror complete phase context pop #8590

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

Merged
merged 1 commit into from
Dec 17, 2016
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
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* provides context when reusing work
* reads context when setState is below the provider
* reads context when setState is above the provider
* maintains the correct context index when context proviers are bailed out due to low priority

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
* catches render error in a boundary during full deferred mounting
Expand Down
13 changes: 11 additions & 2 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -462,8 +462,17 @@ module.exports = function<T, P, I, TI, C, CX>(
}

function bailoutOnLowPriority(current, workInProgress) {
if (workInProgress.tag === HostPortal) {
pushHostContainer(workInProgress.stateNode.containerInfo);
// TODO: Handle HostComponent tags here as well and call pushHostContext()?
// See PR 8590 discussion for context
switch (workInProgress.tag) {
case ClassComponent:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do this I think we should handle HostComponent here too for consistency.
It also gets popped unconditionally in complete phase.
This defensive check is likely why it didn't break so far but we can replace it with an assertion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 from me for replacing with an assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I missed this comment. Let me add an assertion, as you say! 😄

if (isContextProvider(workInProgress)) {
pushContextProvider(workInProgress, false);
}
break;
case HostPortal:
pushHostContainer(workInProgress.stateNode.containerInfo);
break;
}
// TODO: What if this is currently in progress?
// How can that happen? How is this not being cloned?
Expand Down
43 changes: 43 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1932,4 +1932,47 @@ describe('ReactIncremental', () => {
'ShowLocaleFn:read {"locale":"gr"}',
]);
});

it('maintains the correct context index when context proviers are bailed out due to low priority', () => {
class Root extends React.Component {
render() {
return <Middle {...this.props} />;
}
}

let instance;

class Middle extends React.Component {
constructor(props, context) {
super(props, context);
instance = this;
}
shouldComponentUpdate() {
// Return false so that our child will get a NoWork priority (and get bailed out)
return false;
}
render() {
return <Child />;
}
}

// Child must be a context provider to trigger the bug
class Child extends React.Component {
static childContextTypes = {};
getChildContext() {
return {};
}
render() {
return <div />;
}
}

// Init
ReactNoop.render(<Root />);
ReactNoop.flush();

// Trigger an update in the middle of the tree
instance.setState({});
ReactNoop.flush();
});
});