Skip to content

Commit

Permalink
Keep element.ref for now but warn on access
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@acdlite
acdlite committed Feb 20, 2024
1 parent c46fc90 commit 63c9fde
Show file tree
Hide file tree
Showing 8 changed files with 201 additions and 89 deletions.
9 changes: 7 additions & 2 deletions packages/jest-react/src/JestReact.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,20 @@ function assertYieldsWereCleared(root) {
}

function createJSXElementForTestComparison(type, props) {
if (enableRefAsProp) {
return {
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,
Expand Down
58 changes: 34 additions & 24 deletions packages/react-client/src/ReactFlightClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,35 +467,45 @@ 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 = 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,
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.
Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down
11 changes: 8 additions & 3 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
15 changes: 9 additions & 6 deletions packages/react/src/__tests__/ReactCreateElement-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down
11 changes: 8 additions & 3 deletions packages/react/src/__tests__/ReactElementClone-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ describe('ReactElementClone', () => {
let ComponentClass;

beforeEach(() => {
jest.resetModules();

act = require('internal-test-utils').act;

PropTypes = require('prop-types');
Expand Down Expand Up @@ -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);
}
Expand All @@ -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');
Expand All @@ -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);
Expand Down
13 changes: 8 additions & 5 deletions packages/react/src/__tests__/ReactJSXTransformIntegration-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -125,7 +125,10 @@ describe('ReactJSXTransformIntegration', () => {
const element = <Component ref={ref} foo="56" />;
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);
Expand All @@ -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);
}
Expand Down
Loading

0 comments on commit 63c9fde

Please sign in to comment.