Skip to content

Commit bd1b316

Browse files
acdliteacdlite
committed
Keep element.ref for now but warn on access
In the last commit I removed the `ref` property from the element type completely. Instead, let's keep it for another release cycle but warn if it's accessed. In dev, we add a non-enumerable getter with `defineProperty` and warn whenever it's invoked. We don't warn on access if a ref is not given. This reduces false positives in cases where a test serializer uses `getOwnPropertyDescriptors`` to compare objects, like Jest does, which is a problem because it bypasses non-enumerability. So unfortunately this will trigger a false positive warning in Jest when the diff is printed: expect(<div ref={ref} />).toEqual(<span ref={ref} />); A bit sketchy, but this is what we've done for the `props.key` and `props.ref` accessors for years, which implies it will be good enough for `element.ref`, too. Let's see if anyone complains.
1 parent 80e2456 commit bd1b316

File tree

8 files changed

+176
-85
lines changed

8 files changed

+176
-85
lines changed

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,20 @@ function assertYieldsWereCleared(root) {
4040
}
4141

4242
function createJSXElementForTestComparison(type, props) {
43-
if (enableRefAsProp) {
44-
return {
43+
if (__DEV__ && enableRefAsProp) {
44+
const element = {
4545
$$typeof: REACT_ELEMENT_TYPE,
4646
type: type,
4747
key: null,
4848
props: props,
4949
_owner: null,
5050
_store: __DEV__ ? {} : undefined,
5151
};
52+
Object.defineProperty(element, 'ref', {
53+
enumerable: false,
54+
value: null,
55+
});
56+
return element;
5257
} else {
5358
return {
5459
$$typeof: REACT_ELEMENT_TYPE,

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

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -472,30 +472,34 @@ function createElement(
472472
key: mixed,
473473
props: mixed,
474474
): React$Element<any> {
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-
};
475+
let element: any;
476+
if (__DEV__ && enableRefAsProp) {
477+
// `ref` is non-enumerable in dev
478+
element = {
479+
$$typeof: REACT_ELEMENT_TYPE,
480+
type,
481+
key,
482+
props,
483+
_owner: null,
484+
};
485+
Object.defineProperty(element, 'ref', {
486+
enumerable: false,
487+
value: null,
488+
});
489+
} else {
490+
element = {
491+
// This tag allows us to uniquely identify this as a React Element
492+
$$typeof: REACT_ELEMENT_TYPE,
493+
494+
type,
495+
key,
496+
ref: null,
497+
props,
498+
499+
// Record the component responsible for creating this element.
500+
_owner: null,
501+
};
502+
}
499503

500504
if (__DEV__) {
501505
// We don't really need to add any of these but keeping them for good measure.

‎packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,6 @@ describe('ReactDeprecationWarnings', () => {
109109
await waitForAll([]);
110110
});
111111

112-
// @gate !enableRefAsProp || !__DEV__
113112
it('should warn when owner and self are different for string refs', async () => {
114113
class RefComponent extends React.Component {
115114
render() {
@@ -141,7 +140,6 @@ describe('ReactDeprecationWarnings', () => {
141140
});
142141

143142
if (__DEV__) {
144-
// @gate !enableRefAsProp
145143
it('should warn when owner and self are different for string refs', async () => {
146144
class RefComponent extends React.Component {
147145
render() {

‎packages/react-noop-renderer/src/createReactNoop.js

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -783,15 +783,20 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
783783
let currentEventPriority = DefaultEventPriority;
784784

785785
function createJSXElementForTestComparison(type, props) {
786-
if (enableRefAsProp) {
787-
return {
788-
$$typeof: REACT_ELEMENT_TYPE,
786+
if (__DEV__ && enableRefAsProp) {
787+
const element = {
789788
type: type,
789+
$$typeof: REACT_ELEMENT_TYPE,
790790
key: null,
791791
props: props,
792792
_owner: null,
793793
_store: __DEV__ ? {} : undefined,
794794
};
795+
Object.defineProperty(element, 'ref', {
796+
enumerable: false,
797+
value: null,
798+
});
799+
return element;
795800
} else {
796801
return {
797802
$$typeof: REACT_ELEMENT_TYPE,

‎packages/react/src/__tests__/ReactCreateElement-test.js

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ describe('ReactCreateElement', () => {
3838
expect(element.type).toBe(ComponentClass);
3939
expect(element.key).toBe(null);
4040
if (gate(flags => flags.enableRefAsProp)) {
41-
expect(element.ref).toBe(undefined);
41+
expect(element.ref).toBe(null);
4242
} else {
4343
expect(element.ref).toBe(null);
4444
}
@@ -125,7 +125,7 @@ describe('ReactCreateElement', () => {
125125
expect(element.type).toBe('div');
126126
expect(element.key).toBe(null);
127127
if (gate(flags => flags.enableRefAsProp)) {
128-
expect(element.ref).toBe(undefined);
128+
expect(element.ref).toBe(null);
129129
} else {
130130
expect(element.ref).toBe(null);
131131
}
@@ -180,7 +180,10 @@ describe('ReactCreateElement', () => {
180180
});
181181
expect(element.type).toBe(ComponentClass);
182182
if (gate(flags => flags.enableRefAsProp)) {
183-
expect(element.ref).toBe(undefined);
183+
expect(() => expect(element.ref).toBe(ref)).toErrorDev(
184+
'Accessing element.ref is no longer supported',
185+
{withoutStack: true},
186+
);
184187
const expectation = {foo: '56', ref};
185188
Object.freeze(expectation);
186189
expect(element.props).toEqual(expectation);
@@ -216,7 +219,7 @@ describe('ReactCreateElement', () => {
216219
expect(element.type).toBe(ComponentClass);
217220
expect(element.key).toBe(null);
218221
if (gate(flags => flags.enableRefAsProp)) {
219-
expect(element.ref).toBe(undefined);
222+
expect(element.ref).toBe(null);
220223
} else {
221224
expect(element.ref).toBe(null);
222225
}
@@ -232,7 +235,7 @@ describe('ReactCreateElement', () => {
232235
const elementB = React.createElement('div', elementA.props);
233236
expect(elementB.key).toBe(null);
234237
if (gate(flags => flags.enableRefAsProp)) {
235-
expect(elementB.ref).toBe(undefined);
238+
expect(elementB.ref).toBe(null);
236239
} else {
237240
expect(elementB.ref).toBe(null);
238241
}
@@ -246,7 +249,7 @@ describe('ReactCreateElement', () => {
246249
expect(element.type).toBe(ComponentClass);
247250
expect(element.key).toBe('12');
248251
if (gate(flags => flags.enableRefAsProp)) {
249-
expect(element.ref).toBe(undefined);
252+
expect(element.ref).toBe(null);
250253
} else {
251254
expect(element.ref).toBe(null);
252255
}

‎packages/react/src/__tests__/ReactElementClone-test.js

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ describe('ReactElementClone', () => {
1818
let ComponentClass;
1919

2020
beforeEach(() => {
21+
jest.resetModules();
22+
2123
act = require('internal-test-utils').act;
2224

2325
PropTypes = require('prop-types');
@@ -373,7 +375,7 @@ describe('ReactElementClone', () => {
373375
const elementB = React.cloneElement(elementA, elementA.props);
374376
expect(elementB.key).toBe(null);
375377
if (gate(flags => flags.enableRefAsProp)) {
376-
expect(elementB.ref).toBe(undefined);
378+
expect(elementB.ref).toBe(null);
377379
} else {
378380
expect(elementB.ref).toBe(null);
379381
}
@@ -394,8 +396,10 @@ describe('ReactElementClone', () => {
394396
expect(clone.type).toBe(ComponentClass);
395397
expect(clone.key).toBe('12');
396398
if (gate(flags => flags.enableRefAsProp)) {
397-
// `ref` is just a prop so it does override the original ref
398-
expect(clone.props.ref).toBe(undefined);
399+
expect(() => expect(clone.ref).toBe('34')).toErrorDev(
400+
'Accessing element.ref is no longer supported',
401+
{withoutStack: true},
402+
);
399403
} else {
400404
expect(clone.ref).toBe('34');
401405
}
@@ -421,8 +425,7 @@ describe('ReactElementClone', () => {
421425
expect(clone.type).toBe(ComponentClass);
422426
expect(clone.key).toBe('null');
423427
if (gate(flags => flags.enableRefAsProp)) {
424-
// TODO: Remove `ref` field from the element entirely.
425-
expect(clone.ref).toBe(undefined);
428+
expect(clone.ref).toBe(null);
426429
expect(clone.props).toEqual({foo: 'ef', ref: null});
427430
} else {
428431
expect(clone.ref).toBe(null);

‎packages/react/src/__tests__/ReactJSXTransformIntegration-test.js

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ describe('ReactJSXTransformIntegration', () => {
5858
expect(element.type).toBe(Component);
5959
expect(element.key).toBe(null);
6060
if (gate(flags => flags.enableRefAsProp)) {
61-
expect(element.ref).toBe(undefined);
61+
expect(element.ref).toBe(null);
6262
} else {
6363
expect(element.ref).toBe(null);
6464
}
@@ -72,7 +72,7 @@ describe('ReactJSXTransformIntegration', () => {
7272
expect(element.type).toBe('div');
7373
expect(element.key).toBe(null);
7474
if (gate(flags => flags.enableRefAsProp)) {
75-
expect(element.ref).toBe(undefined);
75+
expect(element.ref).toBe(null);
7676
} else {
7777
expect(element.ref).toBe(null);
7878
}
@@ -87,7 +87,7 @@ describe('ReactJSXTransformIntegration', () => {
8787
expect(element.type).toBe('div');
8888
expect(element.key).toBe(null);
8989
if (gate(flags => flags.enableRefAsProp)) {
90-
expect(element.ref).toBe(undefined);
90+
expect(element.ref).toBe(null);
9191
} else {
9292
expect(element.ref).toBe(null);
9393
}
@@ -127,7 +127,10 @@ describe('ReactJSXTransformIntegration', () => {
127127
const element = <Component ref={ref} foo="56" />;
128128
expect(element.type).toBe(Component);
129129
if (gate(flags => flags.enableRefAsProp)) {
130-
expect(element.ref).toBe(undefined);
130+
expect(() => expect(element.ref).toBe(ref)).toErrorDev(
131+
'Accessing element.ref is no longer supported',
132+
{withoutStack: true},
133+
);
131134
const expectation = {foo: '56', ref};
132135
Object.freeze(expectation);
133136
expect(element.props).toEqual(expectation);
@@ -144,7 +147,7 @@ describe('ReactJSXTransformIntegration', () => {
144147
expect(element.type).toBe(Component);
145148
expect(element.key).toBe('12');
146149
if (gate(flags => flags.enableRefAsProp)) {
147-
expect(element.ref).toBe(undefined);
150+
expect(element.ref).toBe(null);
148151
} else {
149152
expect(element.ref).toBe(null);
150153
}

0 commit comments

Comments
 (0)