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.

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

Fix: Class components should "consume" ref prop #28719

Merged
merged 3 commits into from
Apr 3, 2024
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
18 changes: 3 additions & 15 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,29 +261,17 @@ describe('ReactCompositeComponent', () => {
await act(() => {
root.render(<Component ref={refFn1} />);
});
if (gate(flags => flags.enableRefAsProp)) {
expect(instance1.props).toEqual({prop: 'testKey', ref: refFn1});
} else {
expect(instance1.props).toEqual({prop: 'testKey'});
}
expect(instance1.props).toEqual({prop: 'testKey'});

await act(() => {
root.render(<Component ref={refFn2} prop={undefined} />);
});
if (gate(flags => flags.enableRefAsProp)) {
expect(instance2.props).toEqual({prop: 'testKey', ref: refFn2});
} else {
expect(instance2.props).toEqual({prop: 'testKey'});
}
expect(instance2.props).toEqual({prop: 'testKey'});

await act(() => {
root.render(<Component ref={refFn3} prop={null} />);
});
if (gate(flags => flags.enableRefAsProp)) {
expect(instance3.props).toEqual({prop: null, ref: refFn3});
} else {
expect(instance3.props).toEqual({prop: null});
}
expect(instance3.props).toEqual({prop: null});
});

it('should not mutate passed-in props object', async () => {
Expand Down
33 changes: 20 additions & 13 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ import {
mountClassInstance,
resumeMountClassInstance,
updateClassInstance,
resolveClassComponentProps,
} from './ReactFiberClassComponent';
import {resolveDefaultProps} from './ReactFiberLazyComponent';
import {
Expand Down Expand Up @@ -1762,9 +1763,9 @@ function mountLazyComponent(
// Store the unwrapped component in the type.
workInProgress.type = Component;

const resolvedProps = resolveDefaultProps(Component, props);
if (typeof Component === 'function') {
if (isFunctionClassComponent(Component)) {
const resolvedProps = resolveClassComponentProps(Component, props, false);
workInProgress.tag = ClassComponent;
if (__DEV__) {
workInProgress.type = Component =
Expand All @@ -1778,6 +1779,7 @@ function mountLazyComponent(
renderLanes,
);
} else {
const resolvedProps = resolveDefaultProps(Component, props);
workInProgress.tag = FunctionComponent;
if (__DEV__) {
validateFunctionComponentInDev(workInProgress, Component);
Expand All @@ -1795,6 +1797,7 @@ function mountLazyComponent(
} else if (Component !== undefined && Component !== null) {
const $$typeof = Component.$$typeof;
if ($$typeof === REACT_FORWARD_REF_TYPE) {
const resolvedProps = resolveDefaultProps(Component, props);
workInProgress.tag = ForwardRef;
if (__DEV__) {
workInProgress.type = Component =
Expand All @@ -1808,6 +1811,7 @@ function mountLazyComponent(
renderLanes,
);
} else if ($$typeof === REACT_MEMO_TYPE) {
const resolvedProps = resolveDefaultProps(Component, props);
workInProgress.tag = MemoComponent;
return updateMemoComponent(
null,
Expand Down Expand Up @@ -3938,10 +3942,11 @@ function beginWork(
case ClassComponent: {
const Component = workInProgress.type;
const unresolvedProps = workInProgress.pendingProps;
const resolvedProps =
workInProgress.elementType === Component
? unresolvedProps
: resolveDefaultProps(Component, unresolvedProps);
const resolvedProps = resolveClassComponentProps(
Component,
unresolvedProps,
workInProgress.elementType === Component,
);
return updateClassComponent(
current,
workInProgress,
Expand Down Expand Up @@ -4024,10 +4029,11 @@ function beginWork(
}
const Component = workInProgress.type;
const unresolvedProps = workInProgress.pendingProps;
const resolvedProps =
workInProgress.elementType === Component
? unresolvedProps
: resolveDefaultProps(Component, unresolvedProps);
const resolvedProps = resolveClassComponentProps(
Component,
unresolvedProps,
workInProgress.elementType === Component,
);
return mountIncompleteClassComponent(
current,
workInProgress,
Expand All @@ -4042,10 +4048,11 @@ function beginWork(
}
const Component = workInProgress.type;
const unresolvedProps = workInProgress.pendingProps;
const resolvedProps =
workInProgress.elementType === Component
? unresolvedProps
: resolveDefaultProps(Component, unresolvedProps);
const resolvedProps = resolveClassComponentProps(
Component,
unresolvedProps,
workInProgress.elementType === Component,
);
return mountIncompleteFunctionComponent(
current,
workInProgress,
Expand Down
65 changes: 57 additions & 8 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
enableDebugTracing,
enableSchedulingProfiler,
enableLazyContextPropagation,
enableRefAsProp,
} from 'shared/ReactFeatureFlags';
import ReactStrictModeWarnings from './ReactStrictModeWarnings';
import {isMounted} from './ReactFiberTreeReflection';
Expand All @@ -34,7 +35,6 @@ import assign from 'shared/assign';
import isArray from 'shared/isArray';
import {REACT_CONTEXT_TYPE, REACT_CONSUMER_TYPE} from 'shared/ReactSymbols';

import {resolveDefaultProps} from './ReactFiberLazyComponent';
import {
DebugTracingMode,
NoMode,
Expand Down Expand Up @@ -904,7 +904,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;
Expand All @@ -926,6 +931,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.
Expand All @@ -937,7 +949,7 @@ function resumeMountClassInstance(
(typeof instance.UNSAFE_componentWillReceiveProps === 'function' ||
typeof instance.componentWillReceiveProps === 'function')
) {
if (oldProps !== newProps || oldContext !== nextContext) {
if (didReceiveNewProps || oldContext !== nextContext) {
callComponentWillReceiveProps(
workInProgress,
instance,
Expand All @@ -955,7 +967,7 @@ function resumeMountClassInstance(
suspendIfUpdateReadFromEntangledAsyncAction();
newState = workInProgress.memoizedState;
if (
oldProps === newProps &&
!didReceiveNewProps &&
oldState === newState &&
!hasContextChanged() &&
!checkHasForceUpdateAfterProcessing()
Expand Down Expand Up @@ -1052,10 +1064,11 @@ function updateClassInstance(
cloneUpdateQueue(current, workInProgress);

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

Expand Down Expand Up @@ -1225,6 +1238,42 @@ function updateClassInstance(
return shouldUpdate;
}

export function resolveClassComponentProps(
Component: any,
baseProps: Object,
// Only resolve default props if this is a lazy component. Otherwise, they
// would have already been resolved by the JSX runtime.
// TODO: We're going to remove default prop resolution from the JSX runtime
// and keep it only for class components. As part of that change, we should
// remove this extra check.
alreadyResolvedDefaultProps: boolean,
): Object {
let newProps = baseProps;

// Resolve default props. Taken from old JSX runtime, where this used to live.
const defaultProps = Component.defaultProps;
if (defaultProps && !alreadyResolvedDefaultProps) {
newProps = assign({}, newProps, baseProps);
for (const propName in defaultProps) {
if (newProps[propName] === undefined) {
newProps[propName] = defaultProps[propName];
}
}
}

if (enableRefAsProp) {
// Remove ref from the props object, if it exists.
if ('ref' in newProps) {
if (newProps === baseProps) {
newProps = assign({}, newProps);
}
delete newProps.ref;
}
}

return newProps;
}

export {
constructClassInstance,
mountClassInstance,
Expand Down
37 changes: 24 additions & 13 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ import {
setCurrentFiber as setCurrentDebugFiberInDEV,
getCurrentFiber as getCurrentDebugFiberInDEV,
} from './ReactCurrentFiber';
import {resolveDefaultProps} from './ReactFiberLazyComponent';
import {resolveClassComponentProps} from './ReactFiberClassComponent';
import {
isCurrentUpdateNested,
getCommitTime,
Expand Down Expand Up @@ -244,7 +244,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 {
Expand Down Expand Up @@ -471,7 +475,8 @@ function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) {
// TODO: revisit this when we implement resuming.
if (__DEV__) {
if (
finishedWork.type === finishedWork.elementType &&
!finishedWork.type.defaultProps &&
!('ref' in finishedWork.memoizedProps) &&
!didWarnAboutReassigningProps
) {
if (instance.props !== finishedWork.memoizedProps) {
Expand All @@ -497,9 +502,11 @@ function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) {
}
}
const snapshot = instance.getSnapshotBeforeUpdate(
finishedWork.elementType === finishedWork.type
? prevProps
: resolveDefaultProps(finishedWork.type, prevProps),
resolveClassComponentProps(
finishedWork.type,
prevProps,
finishedWork.elementType === finishedWork.type,
),
prevState,
);
if (__DEV__) {
Expand Down Expand Up @@ -807,7 +814,8 @@ function commitClassLayoutLifecycles(
// TODO: revisit this when we implement resuming.
if (__DEV__) {
if (
finishedWork.type === finishedWork.elementType &&
!finishedWork.type.defaultProps &&
!('ref' in finishedWork.memoizedProps) &&
!didWarnAboutReassigningProps
) {
if (instance.props !== finishedWork.memoizedProps) {
Expand Down Expand Up @@ -848,17 +856,19 @@ function commitClassLayoutLifecycles(
}
}
} else {
const prevProps =
finishedWork.elementType === finishedWork.type
? current.memoizedProps
: resolveDefaultProps(finishedWork.type, current.memoizedProps);
const prevProps = resolveClassComponentProps(
finishedWork.type,
current.memoizedProps,
finishedWork.elementType === finishedWork.type,
);
const prevState = current.memoizedState;
// We could update instance props and state here,
// but instead we rely on them being set during last render.
// TODO: revisit this when we implement resuming.
if (__DEV__) {
if (
finishedWork.type === finishedWork.elementType &&
!finishedWork.type.defaultProps &&
Copy link
Member

Choose a reason for hiding this comment

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

This disables the re-assign props warning when defaultProps are present? Seems fine.

!('ref' in finishedWork.memoizedProps) &&
!didWarnAboutReassigningProps
) {
if (instance.props !== finishedWork.memoizedProps) {
Expand Down Expand Up @@ -918,7 +928,8 @@ function commitClassCallbacks(finishedWork: Fiber) {
const instance = finishedWork.stateNode;
if (__DEV__) {
if (
finishedWork.type === finishedWork.elementType &&
!finishedWork.type.defaultProps &&
!('ref' in finishedWork.memoizedProps) &&
!didWarnAboutReassigningProps
) {
if (instance.props !== finishedWork.memoizedProps) {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberLazyComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
import assign from 'shared/assign';

export function resolveDefaultProps(Component: any, baseProps: Object): Object {
// TODO: Remove support for default props for everything except class
// components, including setting default props on a lazy wrapper around a
// class type.

if (Component && Component.defaultProps) {
// Resolve default props. Taken from ReactElement
const props = assign({}, baseProps);
Expand Down
Loading