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 bailout broken in lazy components due to default props resolving #18491

Closed
wants to merge 2 commits into from
Closed

Fix bailout broken in lazy components due to default props resolving #18491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf72db
Add failing tests for lazy components
jddxf Apr 4, 2020
92c15f2
Fix bailout broken in lazy components due to default props resolving
jddxf Apr 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 16 additions & 10 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
View file
Open in desktop
Expand Up @@ -997,11 +997,13 @@ function updateClassInstance(

cloneUpdateQueue(current, workInProgress);

const oldProps = workInProgress.memoizedProps;
instance.props =
const unresolvedOldProps = workInProgress.memoizedProps;
Copy link
Collaborator

@gaearon gaearon Apr 7, 2020 •
edited

Choose a reason for hiding this comment

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

This says they're unresolved. But later we do this:

instance.props = workInProgress.memoizedProps = newProps;

So next time we would have resolved props in memoizedProps? I don't quite follow the logic.

Is there a way we could make it consistent? So that memoizedProps would always be unresolved, for example. Would that work?

Copy link
Contributor Author

@jddxf jddxf Apr 7, 2020 •
edited

Choose a reason for hiding this comment

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

Since we reset workInProgress.memoizedProps to workInProgress.pendingProps right after beginWork, workInProgress.memoizedProps is always unresolved at the start of beginWork.

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Line 1501 in e69ca31

unitOfWork.memoizedProps = unitOfWork.pendingProps;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then can we assign instance.props only? Is there a reason for workInPorgress.memoizedProps assignment? Sorry I'm being dense, it's been a while since I looked a tthis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind. But unfortunately we have to keep their values in sync. Here is the reason.

Copy link
Collaborator

@gaearon gaearon Apr 7, 2020 •
edited

Choose a reason for hiding this comment

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

I still don't think it makes sense to assign either of them.

In the sCU branch, instance.props reassignment makes sense because the passed props actually did change. We want the most recent values inside of them to be accessible to the lifecycle methods, even if sCU "lies" about them not having changed.

In this branch, we've verified props are ===. So the only reason they "changed" is because we resolved default props. That's not a compelling reason to give our components a different props object.

As you noted, memoizedProps gets reset anyway. So it's confusing to assign it here if we know that's gonna happen.

I think we should fix whatever causes the warning instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you change the warning condition to if (shouldUpdate && inst.props !== nextProps) { then the warning won't fire. It will also make sense because the render wasn't called — so the warning message would have been misleading anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

instance.props = workInProgress.memoizedProps = newProps;

This only gives our components a different props object when we've actually resolved default props. But in that case, components are already given a new props object.

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Lines 1001 to 1004 in 2dddd1e

instance.props =
workInProgress.type === workInProgress.elementType
? oldProps
: resolveDefaultProps(workInProgress.type, oldProps);

I think it's ok to remove this assignment if keeping workInProgress.memoizedProps and instance.props stay the same until beginWork finishes is not a requirement. And that seems true now. So let's change the warning condition instead.

const oldProps =
workInProgress.type === workInProgress.elementType
? oldProps
: resolveDefaultProps(workInProgress.type, oldProps);
? unresolvedOldProps
: resolveDefaultProps(workInProgress.type, unresolvedOldProps);
instance.props = oldProps;
const unresolvedNewProps = workInProgress.pendingProps;

const oldContext = instance.context;
const contextType = ctor.contextType;
Expand Down Expand Up @@ -1029,7 +1031,10 @@ function updateClassInstance(
(typeof instance.UNSAFE_componentWillReceiveProps === 'function' ||
typeof instance.componentWillReceiveProps === 'function')
) {
if (oldProps !== newProps || oldContext !== nextContext) {
if (
unresolvedOldProps !== unresolvedNewProps ||
oldContext !== nextContext
) {
callComponentWillReceiveProps(
workInProgress,
instance,
Expand All @@ -1047,7 +1052,7 @@ function updateClassInstance(
newState = workInProgress.memoizedState;

if (
oldProps === newProps &&
unresolvedOldProps === unresolvedNewProps &&
oldState === newState &&
!hasContextChanged() &&
!checkHasForceUpdateAfterProcessing()
Expand All @@ -1056,20 +1061,21 @@ function updateClassInstance(
// effect even though we're bailing out, so that cWU/cDU are called.
if (typeof instance.componentDidUpdate === 'function') {
if (
oldProps !== current.memoizedProps ||
unresolvedOldProps !== current.memoizedProps ||
oldState !== current.memoizedState
) {
workInProgress.effectTag |= Update;
}
}
if (typeof instance.getSnapshotBeforeUpdate === 'function') {
if (
oldProps !== current.memoizedProps ||
unresolvedOldProps !== current.memoizedProps ||
oldState !== current.memoizedState
) {
workInProgress.effectTag |= Snapshot;
}
}
instance.props = workInProgress.memoizedProps = newProps;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, React would warn.

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 859 to 868 in 1fd4543

if (inst.props !== nextProps) {
if (!didWarnAboutReassigningProps) {
console.error(
'It looks like %s is reassigning its own `this.props` while rendering. ' +
'This is not supported and can lead to confusing bugs.',
getComponentName(workInProgress.type) || 'a component',
);
}
didWarnAboutReassigningProps = true;
}

This also mirrors the logic at the end of the same function.

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Lines 1139 to 1151 in 1fd4543

// If shouldComponentUpdate returned false, we should still update the
// memoized props/state to indicate that this work can be reused.
workInProgress.memoizedProps = newProps;
workInProgress.memoizedState = newState;
}
// Update the existing instance's state, props, and context pointers even
// if shouldComponentUpdate returns false.
instance.props = newProps;
instance.state = newState;
instance.context = nextContext;
return shouldUpdate;

This was not needed before because it was already guaranteed by the condition in the branch.

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Line 1050 in 1fd4543

oldProps === newProps &&

return false;
}

Expand Down Expand Up @@ -1121,15 +1127,15 @@ function updateClassInstance(
// effect even though we're bailing out, so that cWU/cDU are called.
if (typeof instance.componentDidUpdate === 'function') {
if (
oldProps !== current.memoizedProps ||
unresolvedOldProps !== current.memoizedProps ||
oldState !== current.memoizedState
) {
workInProgress.effectTag |= Update;
}
}
if (typeof instance.getSnapshotBeforeUpdate === 'function') {
if (
oldProps !== current.memoizedProps ||
unresolvedOldProps !== current.memoizedProps ||
oldState !== current.memoizedState
) {
workInProgress.effectTag |= Snapshot;
Expand Down
91 changes: 91 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js
View file
Open in desktop
Expand Up @@ -343,6 +343,97 @@ describe('ReactLazy', () => {
expect(root).toMatchRenderedOutput('SiblingB');
});

it('resolves defaultProps without breaking bailout due to unchanged props and state, #17151', async () => {
class LazyImpl extends React.Component {
static defaultProps = {value: 0};

render() {
const text = `${this.props.label}: ${this.props.value}`;
return <Text text={text} />;
}
}

const Lazy = lazy(() => fakeImport(LazyImpl));

const instance1 = React.createRef(null);
const instance2 = React.createRef(null);

const root = ReactTestRenderer.create(
<>
<LazyImpl ref={instance1} label="Not lazy" />
<Suspense fallback={<Text text="Loading..." />}>
<Lazy ref={instance2} label="Lazy" />
</Suspense>
</>,
{
unstable_isConcurrent: true,
},
);
expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']);
expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0');

await Promise.resolve();

expect(Scheduler).toFlushAndYield(['Lazy: 0']);
expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

// Should bailout due to unchanged props and state
instance1.current.setState(null);
expect(Scheduler).toFlushAndYield([]);
expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

// Should bailout due to unchanged props and state
instance2.current.setState(null);
expect(Scheduler).toFlushAndYield([]);
expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
});

it('resolves defaultProps without breaking bailout in PureComponent, #17151', async () => {
class LazyImpl extends React.PureComponent {
static defaultProps = {value: 0};
state = {};

render() {
const text = `${this.props.label}: ${this.props.value}`;
return <Text text={text} />;
}
}

const Lazy = lazy(() => fakeImport(LazyImpl));

const instance1 = React.createRef(null);
const instance2 = React.createRef(null);

const root = ReactTestRenderer.create(
<>
<LazyImpl ref={instance1} label="Not lazy" />
<Suspense fallback={<Text text="Loading..." />}>
<Lazy ref={instance2} label="Lazy" />
</Suspense>
</>,
{
unstable_isConcurrent: true,
},
);
expect(Scheduler).toFlushAndYield(['Not lazy: 0', 'Loading...']);
expect(root).not.toMatchRenderedOutput('Not lazy: 0Lazy: 0');

await Promise.resolve();

expect(Scheduler).toFlushAndYield(['Lazy: 0']);
expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

// Should bailout due to shallow equal props and state
instance1.current.setState({});
expect(Scheduler).toFlushAndYield([]);
expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');

// Should bailout due to shallow equal props and state
instance2.current.setState({});
expect(Scheduler).toFlushAndYield([]);
expect(root).toMatchRenderedOutput('Not lazy: 0Lazy: 0');
});

it('sets defaultProps for modern lifecycles', async () => {
class C extends React.Component {
static defaultProps = {text: 'A'};
Expand Down