Skip to content

Commit 856f9ab

Browse files
acdliteacdlite
committed
Pass ref as normal prop
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 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.
1 parent ee607ca commit 856f9ab

27 files changed

+522
-243
lines changed

‎packages/jest-react/src/JestReact.js

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77

88
import {REACT_ELEMENT_TYPE, REACT_FRAGMENT_TYPE} from 'shared/ReactSymbols';
9+
import {enableRefAsProp} from 'shared/ReactFeatureFlags';
910

1011
import isArray from 'shared/isArray';
1112

@@ -38,6 +39,29 @@ function assertYieldsWereCleared(root) {
3839
}
3940
}
4041

42+
function createJSXElementForTestComparison(type, props) {
43+
if (enableRefAsProp) {
44+
return {
45+
$$typeof: REACT_ELEMENT_TYPE,
46+
type: type,
47+
key: null,
48+
props: props,
49+
_owner: null,
50+
_store: __DEV__ ? {} : undefined,
51+
};
52+
} else {
53+
return {
54+
$$typeof: REACT_ELEMENT_TYPE,
55+
type: type,
56+
key: null,
57+
ref: null,
58+
props: props,
59+
_owner: null,
60+
_store: __DEV__ ? {} : undefined,
61+
};
62+
}
63+
}
64+
4165
export function unstable_toMatchRenderedOutput(root, expectedJSX) {
4266
assertYieldsWereCleared(root);
4367
const actualJSON = root.toJSON();
@@ -55,17 +79,9 @@ export function unstable_toMatchRenderedOutput(root, expectedJSX) {
5579
if (actualJSXChildren === null || typeof actualJSXChildren === 'string') {
5680
actualJSX = actualJSXChildren;
5781
} else {
58-
actualJSX = {
59-
$$typeof: REACT_ELEMENT_TYPE,
60-
type: REACT_FRAGMENT_TYPE,
61-
key: null,
62-
ref: null,
63-
props: {
64-
children: actualJSXChildren,
65-
},
66-
_owner: null,
67-
_store: __DEV__ ? {} : undefined,
68-
};
82+
actualJSX = createJSXElementForTestComparison(REACT_FRAGMENT_TYPE, {
83+
children: actualJSXChildren,
84+
});
6985
}
7086
}
7187
} else {
@@ -82,18 +98,12 @@ function jsonChildToJSXChild(jsonChild) {
8298
return jsonChild;
8399
} else {
84100
const jsxChildren = jsonChildrenToJSXChildren(jsonChild.children);
85-
return {
86-
$$typeof: REACT_ELEMENT_TYPE,
87-
type: jsonChild.type,
88-
key: null,
89-
ref: null,
90-
props:
91-
jsxChildren === null
92-
? jsonChild.props
93-
: {...jsonChild.props, children: jsxChildren},
94-
_owner: null,
95-
_store: __DEV__ ? {} : undefined,
96-
};
101+
return createJSXElementForTestComparison(
102+
jsonChild.type,
103+
jsxChildren === null
104+
? jsonChild.props
105+
: {...jsonChild.props, children: jsxChildren},
106+
);
97107
}
98108
}
99109

‎packages/react-client/src/ReactFlightClient.js

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,11 @@ import type {
3535

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

38-
import {enableBinaryFlight, enablePostpone} from 'shared/ReactFeatureFlags';
38+
import {
39+
enableBinaryFlight,
40+
enablePostpone,
41+
enableRefAsProp,
42+
} from 'shared/ReactFeatureFlags';
3943

4044
import {
4145
resolveClientReference,
@@ -468,19 +472,31 @@ function createElement(
468472
key: mixed,
469473
props: mixed,
470474
): React$Element<any> {
471-
const element: any = {
472-
// This tag allows us to uniquely identify this as a React Element
473-
$$typeof: REACT_ELEMENT_TYPE,
474-
475-
// Built-in properties that belong on the element
476-
type: type,
477-
key: key,
478-
ref: null,
479-
props: props,
480-
481-
// Record the component responsible for creating this element.
482-
_owner: null,
483-
};
475+
const element: any = enableRefAsProp
476+
? {
477+
// This tag allows us to uniquely identify this as a React Element
478+
$$typeof: REACT_ELEMENT_TYPE,
479+
480+
// Built-in properties that belong on the element
481+
type,
482+
key,
483+
props,
484+
485+
// Record the component responsible for creating this element.
486+
_owner: null,
487+
}
488+
: {
489+
// Old behavior. When enableRefAsProp is off, `ref` is an extra field.
490+
ref: null,
491+
492+
// Everything else is the same.
493+
$$typeof: REACT_ELEMENT_TYPE,
494+
type,
495+
key,
496+
props,
497+
_owner: null,
498+
};
499+
484500
if (__DEV__) {
485501
// We don't really need to add any of these but keeping them for good measure.
486502
// Unfortunately, _store is enumerable in jest matchers so for equality to

‎packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js

Lines changed: 43 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -753,37 +753,43 @@ describe('Store (legacy)', () => {
753753
`);
754754
});
755755

756-
it('should support expanding deep parts of the tree', () => {
757-
const Wrapper = ({forwardedRef}) => (
758-
<Nested depth={3} forwardedRef={forwardedRef} />
759-
);
760-
const Nested = ({depth, forwardedRef}) =>
761-
depth > 0 ? (
762-
<Nested depth={depth - 1} forwardedRef={forwardedRef} />
763-
) : (
764-
<div ref={forwardedRef} />
756+
// TODO: These tests don't work when enableRefAsProp is on because the
757+
// JSX runtime that's injected into the test environment by the compiler
758+
// is not compatible with older versions of React. Need to configure the
759+
// the test environment in such a way that certain test modules like this
760+
// one can use an older transform.
761+
if (!require('shared/ReactFeatureFlags').enableRefAsProp) {
762+
it('should support expanding deep parts of the tree', () => {
763+
const Wrapper = ({forwardedRef}) => (
764+
<Nested depth={3} forwardedRef={forwardedRef} />
765765
);
766-
767-
let ref = null;
768-
const refSetter = value => {
769-
ref = value;
770-
};
771-
772-
act(() =>
773-
ReactDOM.render(
774-
<Wrapper forwardedRef={refSetter} />,
775-
document.createElement('div'),
776-
),
777-
);
778-
expect(store).toMatchInlineSnapshot(`
766+
const Nested = ({depth, forwardedRef}) =>
767+
depth > 0 ? (
768+
<Nested depth={depth - 1} forwardedRef={forwardedRef} />
769+
) : (
770+
<div ref={forwardedRef} />
771+
);
772+
773+
let ref = null;
774+
const refSetter = value => {
775+
ref = value;
776+
};
777+
778+
act(() =>
779+
ReactDOM.render(
780+
<Wrapper forwardedRef={refSetter} />,
781+
document.createElement('div'),
782+
),
783+
);
784+
expect(store).toMatchInlineSnapshot(`
779785
[root]
780786
▸ <Wrapper>
781787
`);
782788

783-
const deepestedNodeID = global.agent.getIDForNode(ref);
789+
const deepestedNodeID = global.agent.getIDForNode(ref);
784790

785-
act(() => store.toggleIsCollapsed(deepestedNodeID, false));
786-
expect(store).toMatchInlineSnapshot(`
791+
act(() => store.toggleIsCollapsed(deepestedNodeID, false));
792+
expect(store).toMatchInlineSnapshot(`
787793
[root]
788794
▾ <Wrapper>
789795
▾ <Nested>
@@ -793,16 +799,16 @@ describe('Store (legacy)', () => {
793799
<div>
794800
`);
795801

796-
const rootID = store.getElementIDAtIndex(0);
802+
const rootID = store.getElementIDAtIndex(0);
797803

798-
act(() => store.toggleIsCollapsed(rootID, true));
799-
expect(store).toMatchInlineSnapshot(`
804+
act(() => store.toggleIsCollapsed(rootID, true));
805+
expect(store).toMatchInlineSnapshot(`
800806
[root]
801807
▸ <Wrapper>
802808
`);
803809

804-
act(() => store.toggleIsCollapsed(rootID, false));
805-
expect(store).toMatchInlineSnapshot(`
810+
act(() => store.toggleIsCollapsed(rootID, false));
811+
expect(store).toMatchInlineSnapshot(`
806812
[root]
807813
▾ <Wrapper>
808814
▾ <Nested>
@@ -812,17 +818,17 @@ describe('Store (legacy)', () => {
812818
<div>
813819
`);
814820

815-
const id = store.getElementIDAtIndex(1);
821+
const id = store.getElementIDAtIndex(1);
816822

817-
act(() => store.toggleIsCollapsed(id, true));
818-
expect(store).toMatchInlineSnapshot(`
823+
act(() => store.toggleIsCollapsed(id, true));
824+
expect(store).toMatchInlineSnapshot(`
819825
[root]
820826
▾ <Wrapper>
821827
▸ <Nested>
822828
`);
823829

824-
act(() => store.toggleIsCollapsed(id, false));
825-
expect(store).toMatchInlineSnapshot(`
830+
act(() => store.toggleIsCollapsed(id, false));
831+
expect(store).toMatchInlineSnapshot(`
826832
[root]
827833
▾ <Wrapper>
828834
▾ <Nested>
@@ -831,7 +837,8 @@ describe('Store (legacy)', () => {
831837
▾ <Nested>
832838
<div>
833839
`);
834-
});
840+
});
841+
}
835842

836843
it('should support reordering of children', () => {
837844
const Root = ({children}) => <div>{children}</div>;

‎packages/react-dom-bindings/src/client/ReactDOMComponent.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -640,7 +640,9 @@ function setProp(
640640
case 'suppressHydrationWarning':
641641
case 'defaultValue': // Reserved
642642
case 'defaultChecked':
643-
case 'innerHTML': {
643+
case 'innerHTML':
644+
case 'ref': {
645+
// TODO: `ref` is pretty common, should we move it up?
644646
// Noop
645647
break;
646648
}
@@ -988,7 +990,8 @@ function setPropOnCustomElement(
988990
}
989991
case 'suppressContentEditableWarning':
990992
case 'suppressHydrationWarning':
991-
case 'innerHTML': {
993+
case 'innerHTML':
994+
case 'ref': {
992995
// Noop
993996
break;
994997
}
@@ -2194,6 +2197,7 @@ function diffHydratedCustomComponent(
21942197
case 'defaultValue':
21952198
case 'defaultChecked':
21962199
case 'innerHTML':
2200+
case 'ref':
21972201
// Noop
21982202
continue;
21992203
case 'dangerouslySetInnerHTML':
@@ -2307,6 +2311,7 @@ function diffHydratedGenericElement(
23072311
case 'defaultValue':
23082312
case 'defaultChecked':
23092313
case 'innerHTML':
2314+
case 'ref':
23102315
// Noop
23112316
continue;
23122317
case 'dangerouslySetInnerHTML':

‎packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1226,6 +1226,7 @@ function pushAttribute(
12261226
case 'innerHTML': // Must use dangerouslySetInnerHTML instead.
12271227
case 'suppressContentEditableWarning':
12281228
case 'suppressHydrationWarning':
1229+
case 'ref':
12291230
// Ignored. These are built-in to React on the client.
12301231
return;
12311232
case 'autoFocus':
@@ -3391,6 +3392,7 @@ function pushStartCustomElement(
33913392
break;
33923393
case 'suppressContentEditableWarning':
33933394
case 'suppressHydrationWarning':
3395+
case 'ref':
33943396
// Ignored. These are built-in to React on the client.
33953397
break;
33963398
case 'className':
@@ -4964,6 +4966,7 @@ function writeStyleResourceAttributeInJS(
49644966
case 'suppressContentEditableWarning':
49654967
case 'suppressHydrationWarning':
49664968
case 'style':
4969+
case 'ref':
49674970
// Ignored
49684971
return;
49694972

@@ -5157,6 +5160,7 @@ function writeStyleResourceAttributeInAttr(
51575160
case 'suppressContentEditableWarning':
51585161
case 'suppressHydrationWarning':
51595162
case 'style':
5163+
case 'ref':
51605164
// Ignored
51615165
return;
51625166

‎packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,8 @@ function validateProperty(tagName, name, value, eventRegistry) {
186186
case 'suppressHydrationWarning':
187187
case 'defaultValue': // Reserved
188188
case 'defaultChecked':
189-
case 'innerHTML': {
189+
case 'innerHTML':
190+
case 'ref': {
190191
return true;
191192
}
192193
case 'innerText': // Properties

0 commit comments

Comments
 (0)