From e726c0e3e8f8b5a75cd74a6cccf4f65b965d055e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 17 Feb 2024 14:04:09 -0500 Subject: [PATCH] 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(
).toEqual(); 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. --- packages/jest-react/src/JestReact.js | 9 +- .../react-client/src/ReactFlightClient.js | 52 +++--- .../ReactDeprecationWarnings-test.js | 2 - .../src/createReactNoop.js | 11 +- .../src/__tests__/ReactCreateElement-test.js | 15 +- .../src/__tests__/ReactElementClone-test.js | 11 +- .../ReactJSXTransformIntegration-test.js | 13 +- packages/react/src/jsx/ReactJSXElement.js | 171 +++++++++++++----- 8 files changed, 195 insertions(+), 89 deletions(-) diff --git a/packages/jest-react/src/JestReact.js b/packages/jest-react/src/JestReact.js index 47858bf7ef071..f94a035f869b0 100644 --- a/packages/jest-react/src/JestReact.js +++ b/packages/jest-react/src/JestReact.js @@ -40,8 +40,8 @@ function assertYieldsWereCleared(root) { } function createJSXElementForTestComparison(type, props) { - if (enableRefAsProp) { - return { + if (__DEV__ && enableRefAsProp) { + const element = { $$typeof: REACT_ELEMENT_TYPE, type: type, key: null, @@ -49,6 +49,11 @@ function createJSXElementForTestComparison(type, props) { _owner: null, _store: __DEV__ ? {} : undefined, }; + Object.defineProperty(element, 'ref', { + enumerable: false, + value: null, + }); + return element; } else { return { $$typeof: REACT_ELEMENT_TYPE, diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 054de12e6fd19..38983d5746c5e 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -472,30 +472,34 @@ function createElement( key: mixed, props: mixed, ): React$Element { - const element: any = enableRefAsProp - ? { - // 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, - key, - props, - - // Record the component responsible for creating this element. - _owner: null, - } - : { - // Old behavior. When enableRefAsProp is off, `ref` is an extra field. - ref: null, - - // Everything else is the same. - $$typeof: REACT_ELEMENT_TYPE, - type, - key, - props, - _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, + value: null, + }); + } 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. diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js index 63d42ce609d09..4c6a0bed84a6c 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js @@ -109,7 +109,6 @@ describe('ReactDeprecationWarnings', () => { await waitForAll([]); }); - // @gate !enableRefAsProp || !__DEV__ it('should warn when owner and self are different for string refs', async () => { class RefComponent extends React.Component { render() { @@ -141,7 +140,6 @@ describe('ReactDeprecationWarnings', () => { }); if (__DEV__) { - // @gate !enableRefAsProp it('should warn when owner and self are different for string refs', async () => { class RefComponent extends React.Component { render() { diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 8c4f45c426336..77c386457e1d2 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -783,15 +783,20 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { let currentEventPriority = DefaultEventPriority; function createJSXElementForTestComparison(type, props) { - if (enableRefAsProp) { - return { - $$typeof: REACT_ELEMENT_TYPE, + if (__DEV__ && enableRefAsProp) { + const element = { type: type, + $$typeof: REACT_ELEMENT_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, diff --git a/packages/react/src/__tests__/ReactCreateElement-test.js b/packages/react/src/__tests__/ReactCreateElement-test.js index 6c52cc2fb84a5..8eb5f8c0e13f2 100644 --- a/packages/react/src/__tests__/ReactCreateElement-test.js +++ b/packages/react/src/__tests__/ReactCreateElement-test.js @@ -38,7 +38,7 @@ describe('ReactCreateElement', () => { expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } @@ -125,7 +125,7 @@ describe('ReactCreateElement', () => { expect(element.type).toBe('div'); expect(element.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } @@ -180,7 +180,10 @@ describe('ReactCreateElement', () => { }); expect(element.type).toBe(ComponentClass); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(() => expect(element.ref).toBe(ref)).toErrorDev( + 'Accessing element.ref is no longer supported', + {withoutStack: true}, + ); const expectation = {foo: '56', ref}; Object.freeze(expectation); expect(element.props).toEqual(expectation); @@ -216,7 +219,7 @@ describe('ReactCreateElement', () => { expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } @@ -232,7 +235,7 @@ describe('ReactCreateElement', () => { const elementB = React.createElement('div', elementA.props); expect(elementB.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(elementB.ref).toBe(undefined); + expect(elementB.ref).toBe(null); } else { expect(elementB.ref).toBe(null); } @@ -246,7 +249,7 @@ describe('ReactCreateElement', () => { expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index c9a88f4929f4e..2d72859433499 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -18,6 +18,8 @@ describe('ReactElementClone', () => { let ComponentClass; beforeEach(() => { + jest.resetModules(); + act = require('internal-test-utils').act; PropTypes = require('prop-types'); @@ -373,7 +375,7 @@ describe('ReactElementClone', () => { const elementB = React.cloneElement(elementA, elementA.props); expect(elementB.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(elementB.ref).toBe(undefined); + expect(elementB.ref).toBe(null); } else { expect(elementB.ref).toBe(null); } @@ -395,6 +397,10 @@ describe('ReactElementClone', () => { expect(clone.key).toBe('12'); if (gate(flags => flags.enableRefAsProp)) { expect(clone.props.ref).toBe('34'); + expect(() => expect(clone.ref).toBe('34')).toErrorDev( + 'Accessing element.ref is no longer supported', + {withoutStack: true}, + ); expect(clone.props).toEqual({foo: 'ef', ref: '34'}); } else { expect(clone.ref).toBe('34'); @@ -421,8 +427,7 @@ describe('ReactElementClone', () => { expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('null'); if (gate(flags => flags.enableRefAsProp)) { - // TODO: Remove `ref` field from the element entirely. - expect(clone.ref).toBe(undefined); + expect(clone.ref).toBe(null); expect(clone.props).toEqual({foo: 'ef', ref: null}); } else { expect(clone.ref).toBe(null); diff --git a/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js b/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js index fd58114c0b497..19d3458804f84 100644 --- a/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js +++ b/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js @@ -56,7 +56,7 @@ describe('ReactJSXTransformIntegration', () => { expect(element.type).toBe(Component); expect(element.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } @@ -70,7 +70,7 @@ describe('ReactJSXTransformIntegration', () => { expect(element.type).toBe('div'); expect(element.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } @@ -85,7 +85,7 @@ describe('ReactJSXTransformIntegration', () => { expect(element.type).toBe('div'); expect(element.key).toBe(null); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } @@ -125,7 +125,10 @@ describe('ReactJSXTransformIntegration', () => { const element = ; expect(element.type).toBe(Component); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(() => expect(element.ref).toBe(ref)).toErrorDev( + 'Accessing element.ref is no longer supported', + {withoutStack: true}, + ); const expectation = {foo: '56', ref}; Object.freeze(expectation); expect(element.props).toEqual(expectation); @@ -142,7 +145,7 @@ describe('ReactJSXTransformIntegration', () => { expect(element.type).toBe(Component); expect(element.key).toBe('12'); if (gate(flags => flags.enableRefAsProp)) { - expect(element.ref).toBe(undefined); + expect(element.ref).toBe(null); } else { expect(element.ref).toBe(null); } diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index 1ffb7696b8e24..dded86e3e7781 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -31,23 +31,23 @@ const REACT_CLIENT_REFERENCE = Symbol.for('react.client.reference'); let specialPropKeyWarningShown; let specialPropRefWarningShown; let didWarnAboutStringRefs; +let didWarnAboutElementRef; if (__DEV__) { didWarnAboutStringRefs = {}; + didWarnAboutElementRef = {}; } function hasValidRef(config) { - if (!enableRefAsProp) { - if (__DEV__) { - if (hasOwnProperty.call(config, 'ref')) { - const getter = Object.getOwnPropertyDescriptor(config, 'ref').get; - if (getter && getter.isReactWarning) { - return false; - } + if (__DEV__) { + if (hasOwnProperty.call(config, 'ref')) { + const getter = Object.getOwnPropertyDescriptor(config, 'ref').get; + if (getter && getter.isReactWarning) { + return false; } } - return config.ref !== undefined; } + return config.ref !== undefined; } function hasValidKey(config) { @@ -137,6 +137,25 @@ function defineRefPropWarningGetter(props, displayName) { } } +function elementRefGetterWithDeprecationWarning() { + if (__DEV__) { + const componentName = getComponentNameFromType(this.type); + if (!didWarnAboutElementRef[componentName]) { + didWarnAboutElementRef[componentName] = true; + console.error( + 'Accessing element.ref is no longer supported. ref is now a ' + + 'regular prop. It will be removed from the JSX Element ' + + 'type in a future release.', + ); + } + + // An undefined `element.ref` is coerced to `null` for + // backwards compatibility. + const refProp = this.props.ref; + return refProp !== undefined ? refProp : null; + } +} + /** * Factory method to create a new React element. This no longer adheres to * the class pattern, so do not use new to call it. Also, instanceof check @@ -157,31 +176,85 @@ function defineRefPropWarningGetter(props, displayName) { * indicating filename, line number, and/or other information. * @internal */ -function ReactElement(type, key, ref, self, source, owner, props) { - const element = enableRefAsProp - ? { - // 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, - key, - props, - - // Record the component responsible for creating this element. - _owner: owner, - } - : { - // Old behavior. When enableRefAsProp is off, `ref` is an extra field. - ref, - - // Everything else is the same. - $$typeof: REACT_ELEMENT_TYPE, - type, - key, - props, - _owner: owner, - }; +function ReactElement(type, key, _ref, self, source, owner, props) { + let ref; + if (enableRefAsProp) { + // When enableRefAsProp is on, ignore whatever was passed as the ref + // argument and treat `props.ref` as the source of truth. The only thing we + // use this for is `element.ref`, which will log a deprecation warning on + // access. In the next release, we can remove `element.ref` as well as the + // `ref` argument. + const refProp = props.ref; + + // An undefined `element.ref` is coerced to `null` for + // backwards compatibility. + ref = refProp !== undefined ? refProp : null; + } else { + ref = _ref; + } + + let element; + if (__DEV__ && enableRefAsProp) { + // In dev, make `ref` a non-enumerable property with a warning. It's non- + // enumerable so that test matchers and serializers don't access it and + // trigger the warning. + // + // `ref` will be removed from the element completely in a future release. + element = { + // 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, + key, + + props, + + // Record the component responsible for creating this element. + _owner: owner, + }; + if (ref !== null) { + Object.defineProperty(element, 'ref', { + enumerable: false, + get: elementRefGetterWithDeprecationWarning, + }); + } else { + // 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(
).toEqual(); + // + // 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. + Object.defineProperty(element, 'ref', { + enumerable: false, + value: null, + }); + } + } else { + // In prod, `ref` is a regular property. It will be removed in a + // future release. + element = { + // 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, + key, + ref, + + props, + + // Record the component responsible for creating this element. + _owner: owner, + }; + } if (__DEV__) { // The validation flag is currently mutative. We put it on @@ -251,8 +324,10 @@ export function jsxProd(type, config, maybeKey) { key = '' + config.key; } - if (!enableRefAsProp && hasValidRef(config)) { - ref = config.ref; + if (hasValidRef(config)) { + if (!enableRefAsProp) { + ref = config.ref; + } } // Remaining properties are added to a new props object @@ -467,8 +542,10 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { key = '' + config.key; } - if (!enableRefAsProp && hasValidRef(config)) { - ref = config.ref; + if (hasValidRef(config)) { + if (!enableRefAsProp) { + ref = config.ref; + } warnIfStringRefCannotBeAutoConverted(config, self); } @@ -602,8 +679,10 @@ export function createElement(type, config, children) { let ref = null; if (config != null) { - if (!enableRefAsProp && hasValidRef(config)) { - ref = config.ref; + if (hasValidRef(config)) { + if (!enableRefAsProp) { + ref = config.ref; + } if (__DEV__) { warnIfStringRefCannotBeAutoConverted(config, config.__self); @@ -743,7 +822,9 @@ export function cloneAndReplaceKey(oldElement, newKey) { return ReactElement( oldElement.type, newKey, - oldElement.ref, + // When enableRefAsProp is on, this argument is ignored. This check only + // exists to avoid the `ref` access warning. + enableRefAsProp ? null : oldElement.ref, undefined, undefined, oldElement._owner, @@ -769,15 +850,17 @@ export function cloneElement(element, config, children) { // Reserved names are extracted let key = element.key; - let ref = element.ref; + let ref = enableRefAsProp ? null : element.ref; // Owner will be preserved, unless ref is overridden let owner = element._owner; if (config != null) { - if (!enableRefAsProp && hasValidRef(config)) { - // Silently steal the ref from the parent. - ref = config.ref; + if (hasValidRef(config)) { + if (!enableRefAsProp) { + // Silently steal the ref from the parent. + ref = config.ref; + } owner = ReactCurrentOwner.current; } if (hasValidKey(config)) {