Skip to content

Commit

Permalink
Class components should consume the ref prop
Browse files Browse the repository at this point in the history
With the `enableRefAsProp` flag enabled, refs are normal props and no longer filtered in the JSX runtime. Still, some APIs exist that conceptionally "consume" the ref since they bind the ref to a value. This includes `forwardRef` that already implemented filtering the `ref` prop out to the props passed to the inner component. We also need to do the same for class components. A `ref` passed a class component is bound to that class instance, if we keep the ref unfiltered and the component spreads all the props to a child component the `ref` would see 2 or more values set to it.
  • Loading branch information
kassens committed Mar 20, 2024
1 parent a493901 commit 5ae75aa
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 5 deletions.
34 changes: 29 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,25 @@ function updateClassComponent(
nextProps: any,
renderLanes: Lanes,
) {
let nextPropsWithoutRef;
if (enableRefAsProp && 'ref' in nextProps) {
// `ref` is just a prop now, but class components expects it to not appear
// the props object. This used to happen in the JSX runtime, but now we do
// it here. The class "consumes" the ref as it binds the ref to the class
// instance already.
nextPropsWithoutRef = ({}: {[string]: any});
for (const key in nextProps) {
// Since `ref` should only appear in props via the JSX transform, we can
// assume that this is a plain object. So we don't need a
// hasOwnProperty check.
if (key !== 'ref') {
nextPropsWithoutRef[key] = nextProps[key];
}
}
} else {
nextPropsWithoutRef = nextProps;
}

if (__DEV__) {
// This is used by DevTools to force a boundary to error.
switch (shouldError(workInProgress)) {
Expand Down Expand Up @@ -1211,23 +1230,28 @@ function updateClassComponent(
resetSuspendedCurrentOnMountInLegacyMode(current, workInProgress);

// In the initial pass we might need to construct the instance.
constructClassInstance(workInProgress, Component, nextProps);
mountClassInstance(workInProgress, Component, nextProps, renderLanes);
constructClassInstance(workInProgress, Component, nextPropsWithoutRef);
mountClassInstance(
workInProgress,
Component,
nextPropsWithoutRef,
renderLanes,
);
shouldUpdate = true;
} else if (current === null) {
// In a resume, we'll already have an instance we can reuse.
shouldUpdate = resumeMountClassInstance(
workInProgress,
Component,
nextProps,
nextPropsWithoutRef,
renderLanes,
);
} else {
shouldUpdate = updateClassInstance(
current,
workInProgress,
Component,
nextProps,
nextPropsWithoutRef,
renderLanes,
);
}
Expand All @@ -1241,7 +1265,7 @@ function updateClassComponent(
);
if (__DEV__) {
const inst = workInProgress.stateNode;
if (shouldUpdate && inst.props !== nextProps) {
if (shouldUpdate && inst.props !== nextPropsWithoutRef) {
if (!didWarnAboutReassigningProps) {
console.error(
'It looks like %s is reassigning its own `this.props` while rendering. ' +
Expand Down
42 changes: 42 additions & 0 deletions packages/react/src/__tests__/ReactES6Class-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ let PropTypes;
let React;
let ReactDOM;
let ReactDOMClient;
let Scheduler;
let act;
let assertLog;

describe('ReactES6Class', () => {
let container;
Expand All @@ -26,10 +29,12 @@ describe('ReactES6Class', () => {
let renderedName = null;

beforeEach(() => {
({act, assertLog} = require('internal-test-utils'));
PropTypes = require('prop-types');
React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
Scheduler = require('scheduler/unstable_mock');
container = document.createElement('div');
root = ReactDOMClient.createRoot(container);
attachedListener = null;
Expand Down Expand Up @@ -593,6 +598,43 @@ describe('ReactES6Class', () => {
});
}

it('does not pass ref as a prop', async () => {
gate(x => {
console.log('enableRefAsProp', x.enableRefAsProp);
});

class Child extends React.Component {
render() {
Scheduler.log(
`Child rendered with props: ${Object.keys(this.props).join(', ')}`,
);
return null;
}
}

class Parent extends React.Component {
render() {
Scheduler.log(
`Parent rendered with props: ${Object.keys(this.props).join(', ')}`,
);
return <Child {...this.props} />;
}
}

const ref = value => {
Scheduler.log('ref callback called ' + value.constructor.name);
};
await act(async () => {
root.render(<Parent prop1="one" ref={ref} />);
});

assertLog([
'Parent rendered with props: prop1',
'Child rendered with props: prop1',
'ref callback called Parent',
]);
});

it('supports drilling through to the DOM using findDOMNode', () => {
const ref = React.createRef();
test(<Inner name="foo" ref={ref} />, 'DIV', 'foo');
Expand Down

0 comments on commit 5ae75aa

Please sign in to comment.