Skip to content

Commit

Permalink
Fix bailout broken in lazy components due to default props resolving (#…
Browse files Browse the repository at this point in the history
…18539)

* Add failing tests for lazy components

* Fix bailout broken in lazy components due to default props resolving

We should never compare unresolved props with resolved props. Since comparing
resolved props by reference doesn't make sense, we use unresolved props in that
case. Otherwise, resolved props are used.

* Avoid reassigning props warning when we bailout
  • Loading branch information
jddxf committed Apr 8, 2020
1 parent 2dddd1e commit 241103a
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 11 deletions.
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
Expand Up @@ -856,7 +856,7 @@ function updateClassComponent(
);
if (__DEV__) {
const inst = workInProgress.stateNode;
if (inst.props !== nextProps) {
if (shouldUpdate && inst.props !== nextProps) {
if (!didWarnAboutReassigningProps) {
console.error(
'It looks like %s is reassigning its own `this.props` while rendering. ' +
Expand Down
25 changes: 15 additions & 10 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Expand Up @@ -997,11 +997,13 @@ function updateClassInstance(

cloneUpdateQueue(current, workInProgress);

const oldProps = workInProgress.memoizedProps;
instance.props =
const unresolvedOldProps = workInProgress.memoizedProps;
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,15 +1061,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 Expand Up @@ -1121,15 +1126,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
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

0 comments on commit 241103a

Please sign in to comment.