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

Pass ref as normal prop #28348

Merged
merged 4 commits into from
Feb 20, 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
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', {
Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 19, 2024

Choose a reason for hiding this comment

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

Not sure if it helps but it might be worth keeping these as getters that return null so that the object have the same shape whether it's from Flight or JSX.

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