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.
facebook · Apr 3, 2024 · 72e8cf4 · 72e8cf4
1 parent 3c21710
commit 72e8cf4
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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 <Text text={'render: ' + getPropKeys(this.props)} />;
       }
@@ -75,18 +101,33 @@ describe('ReactClassComponentPropResolution', () => {
     await act(async () => {
       root.render(<Component text="Yay" ref={ref} />);
     });
-    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(<Component text="Yay (again)" ref={ref} />);
     });
     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']);
   });
 });