Skip to content

Commit

Permalink
Pass ref as normal prop (#28348)
Browse files Browse the repository at this point in the history
Depends on:

- #28317 
- #28320 

---

Changes the behavior of the JSX runtime to pass through `ref` as a
normal prop, rather than plucking it from the props object and storing
on the element.

This is a breaking change since it changes the type of the receiving
component. However, most code is unaffected since it's unlikely that a
component would have attempted to access a `ref` prop, since it was not
possible to get a reference to one.

`forwardRef` _will_ still pluck `ref` from the props object, though,
since it's extremely common for users to spread the props object onto
the inner component and pass `ref` as a differently named prop. This is
for maximum compatibility with existing code — the real impact of this
change is that `forwardRef` is no longer required.

Currently, refs are resolved during child reconciliation and stored on
the fiber. As a result of this change, we can move ref resolution to
happen only much later, and only for components that actually use them.
Then we can remove the `ref` field from the Fiber type. I have not yet
done that in this step, though.
  • Loading branch information
acdlite authored Feb 20, 2024
1 parent a515d75 commit fa2f82a
Show file tree
Hide file tree
Showing 34 changed files with 672 additions and 243 deletions.
61 changes: 38 additions & 23 deletions packages/jest-react/src/JestReact.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import {REACT_ELEMENT_TYPE, REACT_FRAGMENT_TYPE} from 'shared/ReactSymbols';
import {enableRefAsProp} from 'shared/ReactFeatureFlags';

import isArray from 'shared/isArray';

Expand Down Expand Up @@ -38,6 +39,34 @@ function assertYieldsWereCleared(root) {
}
}

function createJSXElementForTestComparison(type, props) {
if (__DEV__ && enableRefAsProp) {
const element = {
$$typeof: REACT_ELEMENT_TYPE,
type: type,
key: null,
props: props,
_owner: null,
_store: __DEV__ ? {} : undefined,
};
Object.defineProperty(element, 'ref', {
enumerable: false,
value: null,
});
return element;
} else {
return {
$$typeof: REACT_ELEMENT_TYPE,
type: type,
key: null,
ref: null,
props: props,
_owner: null,
_store: __DEV__ ? {} : undefined,
};
}
}

export function unstable_toMatchRenderedOutput(root, expectedJSX) {
assertYieldsWereCleared(root);
const actualJSON = root.toJSON();
Expand All @@ -55,17 +84,9 @@ export function unstable_toMatchRenderedOutput(root, expectedJSX) {
if (actualJSXChildren === null || typeof actualJSXChildren === 'string') {
actualJSX = actualJSXChildren;
} else {
actualJSX = {
$$typeof: REACT_ELEMENT_TYPE,
type: REACT_FRAGMENT_TYPE,
key: null,
ref: null,
props: {
children: actualJSXChildren,
},
_owner: null,
_store: __DEV__ ? {} : undefined,
};
actualJSX = createJSXElementForTestComparison(REACT_FRAGMENT_TYPE, {
children: actualJSXChildren,
});
}
}
} else {
Expand All @@ -82,18 +103,12 @@ function jsonChildToJSXChild(jsonChild) {
return jsonChild;
} else {
const jsxChildren = jsonChildrenToJSXChildren(jsonChild.children);
return {
$$typeof: REACT_ELEMENT_TYPE,
type: jsonChild.type,
key: null,
ref: null,
props:
jsxChildren === null
? jsonChild.props
: {...jsonChild.props, children: jsxChildren},
_owner: null,
_store: __DEV__ ? {} : undefined,
};
return createJSXElementForTestComparison(
jsonChild.type,
jsxChildren === null
? jsonChild.props
: {...jsonChild.props, children: jsxChildren},
);
}
}

Expand Down
54 changes: 40 additions & 14 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ import type {

import type {Postpone} from 'react/src/ReactPostpone';

import {enableBinaryFlight, enablePostpone} from 'shared/ReactFeatureFlags';
import {
enableBinaryFlight,
enablePostpone,
enableRefAsProp,
} from 'shared/ReactFeatureFlags';

import {
resolveClientReference,
Expand Down Expand Up @@ -463,24 +467,46 @@ export function reportGlobalError(response: Response, error: Error): void {
});
}

function nullRefGetter() {
if (__DEV__) {
return null;
}
}

function createElement(
type: mixed,
key: mixed,
props: mixed,
): React$Element<any> {
const element: any = {
// This tag allows us to uniquely identify this as a React Element
$$typeof: REACT_ELEMENT_TYPE,

// Built-in properties that belong on the element
type: type,
key: key,
ref: null,
props: props,

// Record the component responsible for creating this element.
_owner: null,
};
let element: any;
if (__DEV__ && enableRefAsProp) {
// `ref` is non-enumerable in dev
element = ({
$$typeof: REACT_ELEMENT_TYPE,
type,
key,
props,
_owner: null,
}: any);
Object.defineProperty(element, 'ref', {
enumerable: false,
get: nullRefGetter,
});
} else {
element = ({
// This tag allows us to uniquely identify this as a React Element
$$typeof: REACT_ELEMENT_TYPE,

type,
key,
ref: null,
props,

// Record the component responsible for creating this element.
_owner: null,
}: any);
}

if (__DEV__) {
// We don't really need to add any of these but keeping them for good measure.
// Unfortunately, _store is enumerable in jest matchers so for equality to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -753,37 +753,43 @@ describe('Store (legacy)', () => {
`);
});

it('should support expanding deep parts of the tree', () => {
const Wrapper = ({forwardedRef}) => (
<Nested depth={3} forwardedRef={forwardedRef} />
);
const Nested = ({depth, forwardedRef}) =>
depth > 0 ? (
<Nested depth={depth - 1} forwardedRef={forwardedRef} />
) : (
<div ref={forwardedRef} />
// TODO: These tests don't work when enableRefAsProp is on because the
// JSX runtime that's injected into the test environment by the compiler
// is not compatible with older versions of React. Need to configure the
// the test environment in such a way that certain test modules like this
// one can use an older transform.
if (!require('shared/ReactFeatureFlags').enableRefAsProp) {
it('should support expanding deep parts of the tree', () => {
const Wrapper = ({forwardedRef}) => (
<Nested depth={3} forwardedRef={forwardedRef} />
);

let ref = null;
const refSetter = value => {
ref = value;
};

act(() =>
ReactDOM.render(
<Wrapper forwardedRef={refSetter} />,
document.createElement('div'),
),
);
expect(store).toMatchInlineSnapshot(`
const Nested = ({depth, forwardedRef}) =>
depth > 0 ? (
<Nested depth={depth - 1} forwardedRef={forwardedRef} />
) : (
<div ref={forwardedRef} />
);

let ref = null;
const refSetter = value => {
ref = value;
};

act(() =>
ReactDOM.render(
<Wrapper forwardedRef={refSetter} />,
document.createElement('div'),
),
);
expect(store).toMatchInlineSnapshot(`
[root]
▸ <Wrapper>
`);

const deepestedNodeID = global.agent.getIDForNode(ref);
const deepestedNodeID = global.agent.getIDForNode(ref);

act(() => store.toggleIsCollapsed(deepestedNodeID, false));
expect(store).toMatchInlineSnapshot(`
act(() => store.toggleIsCollapsed(deepestedNodeID, false));
expect(store).toMatchInlineSnapshot(`
[root]
▾ <Wrapper>
▾ <Nested>
Expand All @@ -793,16 +799,16 @@ describe('Store (legacy)', () => {
<div>
`);

const rootID = store.getElementIDAtIndex(0);
const rootID = store.getElementIDAtIndex(0);

act(() => store.toggleIsCollapsed(rootID, true));
expect(store).toMatchInlineSnapshot(`
act(() => store.toggleIsCollapsed(rootID, true));
expect(store).toMatchInlineSnapshot(`
[root]
▸ <Wrapper>
`);

act(() => store.toggleIsCollapsed(rootID, false));
expect(store).toMatchInlineSnapshot(`
act(() => store.toggleIsCollapsed(rootID, false));
expect(store).toMatchInlineSnapshot(`
[root]
▾ <Wrapper>
▾ <Nested>
Expand All @@ -812,17 +818,17 @@ describe('Store (legacy)', () => {
<div>
`);

const id = store.getElementIDAtIndex(1);
const id = store.getElementIDAtIndex(1);

act(() => store.toggleIsCollapsed(id, true));
expect(store).toMatchInlineSnapshot(`
act(() => store.toggleIsCollapsed(id, true));
expect(store).toMatchInlineSnapshot(`
[root]
▾ <Wrapper>
▸ <Nested>
`);

act(() => store.toggleIsCollapsed(id, false));
expect(store).toMatchInlineSnapshot(`
act(() => store.toggleIsCollapsed(id, false));
expect(store).toMatchInlineSnapshot(`
[root]
▾ <Wrapper>
▾ <Nested>
Expand All @@ -831,7 +837,8 @@ describe('Store (legacy)', () => {
▾ <Nested>
<div>
`);
});
});
}

it('should support reordering of children', () => {
const Root = ({children}) => <div>{children}</div>;
Expand Down
9 changes: 7 additions & 2 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,9 @@ function setProp(
case 'suppressHydrationWarning':
case 'defaultValue': // Reserved
case 'defaultChecked':
case 'innerHTML': {
case 'innerHTML':
case 'ref': {
// TODO: `ref` is pretty common, should we move it up?
// Noop
break;
}
Expand Down Expand Up @@ -988,7 +990,8 @@ function setPropOnCustomElement(
}
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'innerHTML': {
case 'innerHTML':
case 'ref': {
// Noop
break;
}
Expand Down Expand Up @@ -2194,6 +2197,7 @@ function diffHydratedCustomComponent(
case 'defaultValue':
case 'defaultChecked':
case 'innerHTML':
case 'ref':
// Noop
continue;
case 'dangerouslySetInnerHTML':
Expand Down Expand Up @@ -2307,6 +2311,7 @@ function diffHydratedGenericElement(
case 'defaultValue':
case 'defaultChecked':
case 'innerHTML':
case 'ref':
// Noop
continue;
case 'dangerouslySetInnerHTML':
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,7 @@ function pushAttribute(
case 'innerHTML': // Must use dangerouslySetInnerHTML instead.
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'ref':
// Ignored. These are built-in to React on the client.
return;
case 'autoFocus':
Expand Down Expand Up @@ -3391,6 +3392,7 @@ function pushStartCustomElement(
break;
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'ref':
// Ignored. These are built-in to React on the client.
break;
case 'className':
Expand Down Expand Up @@ -4964,6 +4966,7 @@ function writeStyleResourceAttributeInJS(
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'style':
case 'ref':
// Ignored
return;

Expand Down Expand Up @@ -5157,6 +5160,7 @@ function writeStyleResourceAttributeInAttr(
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'style':
case 'ref':
// Ignored
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ function validateProperty(tagName, name, value, eventRegistry) {
case 'suppressHydrationWarning':
case 'defaultValue': // Reserved
case 'defaultChecked':
case 'innerHTML': {
case 'innerHTML':
case 'ref': {
return true;
}
case 'innerText': // Properties
Expand Down
Loading

0 comments on commit fa2f82a

Please sign in to comment.