Skip to content

Commit aa4cec5

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 a54df41 commit aa4cec5

File tree

8 files changed

+195
-89
lines changed

8 files changed

+195
-89
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+
}: any);
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+
}: any);
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 & 3 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
}
@@ -399,6 +401,10 @@ describe('ReactElementClone', () => {
399401
// distinguish between "has a ref property of undefined" versus "does
400402
// not have ref property"?
401403
expect(clone.props.ref).toBe('34');
404+
expect(() => expect(clone.ref).toBe('34')).toErrorDev(
405+
'Accessing element.ref is no longer supported',
406+
{withoutStack: true},
407+
);
402408
expect(clone.props).toEqual({foo: 'ef', ref: '34'});
403409
} else {
404410
expect(clone.ref).toBe('34');
@@ -425,8 +431,7 @@ describe('ReactElementClone', () => {
425431
expect(clone.type).toBe(ComponentClass);
426432
expect(clone.key).toBe('null');
427433
if (gate(flags => flags.enableRefAsProp)) {
428-
// TODO: Remove `ref` field from the element entirely.
429-
expect(clone.ref).toBe(undefined);
434+
expect(clone.ref).toBe(null);
430435
expect(clone.props).toEqual({foo: 'ef', ref: null});
431436
} else {
432437
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
@@ -56,7 +56,7 @@ describe('ReactJSXTransformIntegration', () => {
5656
expect(element.type).toBe(Component);
5757
expect(element.key).toBe(null);
5858
if (gate(flags => flags.enableRefAsProp)) {
59-
expect(element.ref).toBe(undefined);
59+
expect(element.ref).toBe(null);
6060
} else {
6161
expect(element.ref).toBe(null);
6262
}
@@ -70,7 +70,7 @@ describe('ReactJSXTransformIntegration', () => {
7070
expect(element.type).toBe('div');
7171
expect(element.key).toBe(null);
7272
if (gate(flags => flags.enableRefAsProp)) {
73-
expect(element.ref).toBe(undefined);
73+
expect(element.ref).toBe(null);
7474
} else {
7575
expect(element.ref).toBe(null);
7676
}
@@ -85,7 +85,7 @@ describe('ReactJSXTransformIntegration', () => {
8585
expect(element.type).toBe('div');
8686
expect(element.key).toBe(null);
8787
if (gate(flags => flags.enableRefAsProp)) {
88-
expect(element.ref).toBe(undefined);
88+
expect(element.ref).toBe(null);
8989
} else {
9090
expect(element.ref).toBe(null);
9191
}
@@ -125,7 +125,10 @@ describe('ReactJSXTransformIntegration', () => {
125125
const element = <Component ref={ref} foo="56" />;
126126
expect(element.type).toBe(Component);
127127
if (gate(flags => flags.enableRefAsProp)) {
128-
expect(element.ref).toBe(undefined);
128+
expect(() => expect(element.ref).toBe(ref)).toErrorDev(
129+
'Accessing element.ref is no longer supported',
130+
{withoutStack: true},
131+
);
129132
const expectation = {foo: '56', ref};
130133
Object.freeze(expectation);
131134
expect(element.props).toEqual(expectation);
@@ -142,7 +145,7 @@ describe('ReactJSXTransformIntegration', () => {
142145
expect(element.type).toBe(Component);
143146
expect(element.key).toBe('12');
144147
if (gate(flags => flags.enableRefAsProp)) {
145-
expect(element.ref).toBe(undefined);
148+
expect(element.ref).toBe(null);
146149
} else {
147150
expect(element.ref).toBe(null);
148151
}

0 commit comments

Comments
 (0)