From 72e8cf4feb280e3d01b7ceda3e57d11ffd88b897 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Apr 2024 22:23:30 -0400 Subject: [PATCH] Fix places where class props aren't resolved Noticed these once I enabled class prop resolution in more places. Technically this was already observable if you wrapped a class with `React.lazy` and gave that wrapper default props, but since that was so rare it was never reported. --- .../src/ReactFiberClassComponent.js | 18 ++++++-- .../src/ReactFiberCommitWork.js | 6 ++- .../ReactClassComponentPropResolution-test.js | 43 ++++++++++++++++++- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index bd0caf1e42e96..ef99ed130cd97 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -908,7 +908,12 @@ function resumeMountClassInstance( ): boolean { const instance = workInProgress.stateNode; - const oldProps = workInProgress.memoizedProps; + const unresolvedOldProps = workInProgress.memoizedProps; + const oldProps = resolveClassComponentProps( + ctor, + unresolvedOldProps, + workInProgress.type === workInProgress.elementType, + ); instance.props = oldProps; const oldContext = instance.context; @@ -930,6 +935,13 @@ function resumeMountClassInstance( typeof getDerivedStateFromProps === 'function' || typeof instance.getSnapshotBeforeUpdate === 'function'; + // When comparing whether props changed, we should compare using the + // unresolved props object that is stored on the fiber, rather than the + // one that gets assigned to the instance, because that object may have been + // cloned to resolve default props and/or remove `ref`. + const unresolvedNewProps = workInProgress.pendingProps; + const didReceiveNewProps = unresolvedNewProps !== unresolvedOldProps; + // Note: During these life-cycles, instance.props/instance.state are what // ever the previously attempted to render - not the "current". However, // during componentDidUpdate we pass the "current" props. @@ -941,7 +953,7 @@ function resumeMountClassInstance( (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || typeof instance.componentWillReceiveProps === 'function') ) { - if (oldProps !== newProps || oldContext !== nextContext) { + if (didReceiveNewProps || oldContext !== nextContext) { callComponentWillReceiveProps( workInProgress, instance, @@ -959,7 +971,7 @@ function resumeMountClassInstance( suspendIfUpdateReadFromEntangledAsyncAction(); newState = workInProgress.memoizedState; if ( - oldProps === newProps && + didReceiveNewProps && oldState === newState && !hasContextChanged() && !checkHasForceUpdateAfterProcessing() diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 458e900601971..848f0bf0b4154 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -243,7 +243,11 @@ function shouldProfile(current: Fiber): boolean { } function callComponentWillUnmountWithTimer(current: Fiber, instance: any) { - instance.props = current.memoizedProps; + instance.props = resolveClassComponentProps( + current.type, + current.memoizedProps, + current.elementType === current.type, + ); instance.state = current.memoizedState; if (shouldProfile(current)) { try { diff --git a/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js b/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js index 576c6991af065..53f3a3d04b564 100644 --- a/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js +++ b/packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js @@ -39,6 +39,10 @@ describe('ReactClassComponentPropResolution', () => { } class Component extends React.Component { + constructor(props) { + super(props); + Scheduler.log('constructor: ' + getPropKeys(props)); + } shouldComponentUpdate(props) { Scheduler.log( 'shouldComponentUpdate (prev props): ' + getPropKeys(this.props), @@ -59,6 +63,28 @@ describe('ReactClassComponentPropResolution', () => { Scheduler.log('componentDidMount: ' + getPropKeys(this.props)); return true; } + UNSAFE_componentWillMount() { + Scheduler.log('componentWillMount: ' + getPropKeys(this.props)); + } + UNSAFE_componentWillReceiveProps(nextProps) { + Scheduler.log( + 'componentWillReceiveProps (prev props): ' + getPropKeys(this.props), + ); + Scheduler.log( + 'componentWillReceiveProps (next props): ' + getPropKeys(nextProps), + ); + } + UNSAFE_componentWillUpdate(nextProps) { + Scheduler.log( + 'componentWillUpdate (prev props): ' + getPropKeys(this.props), + ); + Scheduler.log( + 'componentWillUpdate (next props): ' + getPropKeys(nextProps), + ); + } + componentWillUnmount() { + Scheduler.log('componentWillUnmount: ' + getPropKeys(this.props)); + } render() { return ; } @@ -75,18 +101,33 @@ describe('ReactClassComponentPropResolution', () => { await act(async () => { root.render(); }); - assertLog(['render: text, default', 'componentDidMount: text, default']); + assertLog([ + 'constructor: text, default', + 'componentWillMount: text, default', + 'render: text, default', + 'componentDidMount: text, default', + ]); // Update await act(async () => { root.render(); }); assertLog([ + 'componentWillReceiveProps (prev props): text, default', + 'componentWillReceiveProps (next props): text, default', 'shouldComponentUpdate (prev props): text, default', 'shouldComponentUpdate (next props): text, default', + 'componentWillUpdate (prev props): text, default', + 'componentWillUpdate (next props): text, default', 'render: text, default', 'componentDidUpdate (prev props): text, default', 'componentDidUpdate (next props): text, default', ]); + + // Unmount + await act(async () => { + root.render(null); + }); + assertLog(['componentWillUnmount: text, default']); }); });