Skip to content
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();
});
});