Skip to content

Commit

Permalink
Pass ref as normal prop
Browse files Browse the repository at this point in the history
  • Loading branch information
acdlite committed Feb 15, 2024
1 parent 1b9c328 commit 20e5daf
Show file tree
Hide file tree
Showing 22 changed files with 350 additions and 128 deletions.
47 changes: 33 additions & 14 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ import type {

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

import {enableBinaryFlight, enablePostpone} from 'shared/ReactFeatureFlags';
import {
enableBinaryFlight,
enablePostpone,
enableFastJSX,
} from 'shared/ReactFeatureFlags';

import {
resolveClientReference,
Expand Down Expand Up @@ -468,19 +472,34 @@ function createElement(
key: mixed,
props: mixed,
): React$Element<any> {
const element: any = {
// 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: type,
key: key,
ref: null,
props: props,

// Record the component responsible for creating this element.
_owner: null,
};
const element: any = enableFastJSX
? {
// TODO: Remove this field
ref: null,

// 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 enableFastJSX is off, `ref` is an extra field.
ref: null,

// Everything else is the same.
$$typeof: REACT_ELEMENT_TYPE,
type,
key,
props,
_owner: null,
};

if (__DEV__) {
// We don't really need to add any of these but keeping them for good measure.
// Unfortunately, _store is enumerable in jest matchers so for equality to
Expand Down
9 changes: 7 additions & 2 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,9 @@ function setProp(
case 'suppressHydrationWarning':
case 'defaultValue': // Reserved
case 'defaultChecked':
case 'innerHTML': {
case 'innerHTML':
case 'ref': {
// TODO: `ref` is pretty common, should we move it up?
// Noop
break;
}
Expand Down Expand Up @@ -988,7 +990,8 @@ function setPropOnCustomElement(
}
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'innerHTML': {
case 'innerHTML':
case 'ref': {
// Noop
break;
}
Expand Down Expand Up @@ -2194,6 +2197,7 @@ function diffHydratedCustomComponent(
case 'defaultValue':
case 'defaultChecked':
case 'innerHTML':
case 'ref':
// Noop
continue;
case 'dangerouslySetInnerHTML':
Expand Down Expand Up @@ -2307,6 +2311,7 @@ function diffHydratedGenericElement(
case 'defaultValue':
case 'defaultChecked':
case 'innerHTML':
case 'ref':
// Noop
continue;
case 'dangerouslySetInnerHTML':
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,7 @@ function pushAttribute(
case 'innerHTML': // Must use dangerouslySetInnerHTML instead.
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'ref':
// Ignored. These are built-in to React on the client.
return;
case 'autoFocus':
Expand Down Expand Up @@ -3391,6 +3392,7 @@ function pushStartCustomElement(
break;
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'ref':
// Ignored. These are built-in to React on the client.
break;
case 'className':
Expand Down Expand Up @@ -4964,6 +4966,7 @@ function writeStyleResourceAttributeInJS(
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'style':
case 'ref':
// Ignored
return;

Expand Down Expand Up @@ -5157,6 +5160,7 @@ function writeStyleResourceAttributeInAttr(
case 'suppressContentEditableWarning':
case 'suppressHydrationWarning':
case 'style':
case 'ref':
// Ignored
return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ function validateProperty(tagName, name, value, eventRegistry) {
case 'suppressHydrationWarning':
case 'defaultValue': // Reserved
case 'defaultChecked':
case 'innerHTML': {
case 'innerHTML':
case 'ref': {
return true;
}
case 'innerText': // Properties
Expand Down
38 changes: 30 additions & 8 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,26 +278,48 @@ describe('ReactCompositeComponent', () => {
}
}

function refFn1(ref) {
instance1 = ref;
}

function refFn2(ref) {
instance2 = ref;
}

function refFn3(ref) {
instance3 = ref;
}

let instance1;
let instance2;
let instance3;
const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => {
root.render(<Component ref={ref => (instance1 = ref)} />);
root.render(<Component ref={refFn1} />);
});
expect(instance1.props).toEqual({prop: 'testKey'});
if (gate(flags => flags.enableFastJSX)) {
expect(instance1.props).toEqual({prop: 'testKey', ref: refFn1});
} else {
expect(instance1.props).toEqual({prop: 'testKey'});
}

await act(() => {
root.render(
<Component ref={ref => (instance2 = ref)} prop={undefined} />,
);
root.render(<Component ref={refFn2} prop={undefined} />);
});
expect(instance2.props).toEqual({prop: 'testKey'});
if (gate(flags => flags.enableFastJSX)) {
expect(instance2.props).toEqual({prop: 'testKey', ref: refFn2});
} else {
expect(instance2.props).toEqual({prop: 'testKey'});
}

await act(() => {
root.render(<Component ref={ref => (instance3 = ref)} prop={null} />);
root.render(<Component ref={refFn3} prop={null} />);
});
expect(instance3.props).toEqual({prop: null});
if (gate(flags => flags.enableFastJSX)) {
expect(instance3.props).toEqual({prop: null, ref: refFn3});
} else {
expect(instance3.props).toEqual({prop: null});
}
});

it('should not mutate passed-in props object', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ describe('ReactDeprecationWarnings', () => {
await waitForAll([]);
});

// @gate !enableFastJSX
it('should warn when owner and self are different for string refs', async () => {
class RefComponent extends React.Component {
render() {
Expand Down Expand Up @@ -140,6 +141,7 @@ describe('ReactDeprecationWarnings', () => {
});

if (__DEV__) {
// @gate !enableFastJSX
it('should warn when owner and self are different for string refs', async () => {
class RefComponent extends React.Component {
render() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ describe('ReactFunctionComponent', () => {
);
});

// @gate !enableFastJSX
it('should warn when given a string ref', () => {
function Indirection(props) {
return <div>{props.children}</div>;
Expand Down Expand Up @@ -226,6 +227,7 @@ describe('ReactFunctionComponent', () => {
ReactTestUtils.renderIntoDocument(<ParentUsingStringRef />);
});

// @gate !enableFastJSX
it('should warn when given a function ref', () => {
function Indirection(props) {
return <div>{props.children}</div>;
Expand Down Expand Up @@ -264,6 +266,7 @@ describe('ReactFunctionComponent', () => {
ReactTestUtils.renderIntoDocument(<ParentUsingFunctionRef />);
});

// @gate !enableFastJSX
it('deduplicates ref warnings based on element or owner', () => {
// When owner uses JSX, we can use exact line location to dedupe warnings
class AnonymousParentUsingJSX extends React.Component {
Expand Down Expand Up @@ -327,6 +330,7 @@ describe('ReactFunctionComponent', () => {
// This guards against a regression caused by clearing the current debug fiber.
// https://github.com/facebook/react/issues/10831
// @gate !disableLegacyContext || !__DEV__
// @gate !enableFastJSX
it('should warn when giving a function ref with context', () => {
function Child() {
return null;
Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/__tests__/refs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,6 @@ describe('ref swapping', () => {
type: 'div',
props: {},
key: null,
ref: null,
});
});

Expand All @@ -428,9 +427,10 @@ describe('ref swapping', () => {
root.render({
$$typeof: Symbol.for('react.element'),
type: 'div',
props: {},
props: {
ref: NaN,
},
key: null,
ref: undefined,
});
});
}).rejects.toThrow(
Expand Down
15 changes: 14 additions & 1 deletion packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
import {ClassComponent, HostText, HostPortal, Fragment} from './ReactWorkTags';
import isArray from 'shared/isArray';
import {checkPropStringCoercion} from 'shared/CheckStringCoercion';
import {enableFastJSX} from 'shared/ReactFeatureFlags';

import {
createWorkInProgress,
Expand Down Expand Up @@ -145,7 +146,19 @@ function coerceRef(
current: Fiber | null,
element: ReactElement,
) {
const mixedRef = element.ref;
let mixedRef;
if (enableFastJSX) {
// TODO: This is a temporary, intermediate step. When enableFastJSX is on,
// we should resolve the `ref` prop during the begin phase of the component
// it's attached to (HostComponent, ClassComponent, etc).

const refProp = element.props.ref;
mixedRef = refProp !== undefined ? refProp : null;
} else {
// Old behavior.
mixedRef = element.ref;
}

if (
mixedRef !== null &&
typeof mixedRef !== 'function' &&
Expand Down
25 changes: 22 additions & 3 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ import {
enableAsyncActions,
enablePostpone,
enableRenderableContext,
enableFastJSX,
} from 'shared/ReactFeatureFlags';
import isArray from 'shared/isArray';
import shallowEqual from 'shared/shallowEqual';
Expand Down Expand Up @@ -421,6 +422,24 @@ function updateForwardRef(
const render = Component.render;
const ref = workInProgress.ref;

let propsWithoutRef;
if (enableFastJSX && 'ref' in nextProps) {
// `ref` is just a prop now, but `forwardRef` expects it to not appear in
// the props object. This used to happen in the JSX runtime, but now we do
// it here.
propsWithoutRef = {};
for (const key in nextProps) {
// Since `ref` should only appear in props via the JSX transform, we can
// assume that this is a plain object. So we don't need a
// hasOwnProperty check.
if (key !== 'ref') {
propsWithoutRef[key] = nextProps[key];
}
}
} else {
propsWithoutRef = nextProps;
}

// The rest is a fork of updateFunctionComponent
let nextChildren;
let hasId;
Expand All @@ -435,7 +454,7 @@ function updateForwardRef(
current,
workInProgress,
render,
nextProps,
propsWithoutRef,
ref,
renderLanes,
);
Expand All @@ -446,7 +465,7 @@ function updateForwardRef(
current,
workInProgress,
render,
nextProps,
propsWithoutRef,
ref,
renderLanes,
);
Expand Down Expand Up @@ -2095,7 +2114,7 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) {
);
}
}
if (workInProgress.ref !== null) {
if (!enableFastJSX && workInProgress.ref !== null) {
let info = '';
const componentName = getComponentNameFromType(Component) || 'Unknown';
const ownerName = getCurrentFiberOwnerNameInDevOrNull();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1296,16 +1296,22 @@ describe('ReactIncrementalSideEffects', () => {
}

ReactNoop.render(<Foo show={true} />);
await expect(async () => await waitForAll([])).toErrorDev(
'Warning: Function components cannot be given refs. ' +
'Attempts to access this ref will fail. ' +
'Did you mean to use React.forwardRef()?\n\n' +
'Check the render method ' +
'of `Foo`.\n' +
' in FunctionComponent (at **)\n' +
' in div (at **)\n' +
' in Foo (at **)',
);

if (gate(flags => flags.enableFastJSX)) {
await waitForAll([]);
} else {
await expect(async () => await waitForAll([])).toErrorDev(
'Warning: Function components cannot be given refs. ' +
'Attempts to access this ref will fail. ' +
'Did you mean to use React.forwardRef()?\n\n' +
'Check the render method ' +
'of `Foo`.\n' +
' in FunctionComponent (at **)\n' +
' in div (at **)\n' +
' in Foo (at **)',
);
}

expect(ops).toEqual([
classInstance,
// no call for function components
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,7 @@ describe('ReactLazy', () => {
expect(root).toMatchRenderedOutput('2');
});

// @gate !enableFastJSX
it('warns about ref on functions for lazy-loaded components', async () => {
const Foo = props => <div />;
const LazyFoo = lazy(() => {
Expand Down
Loading

0 comments on commit 20e5daf

Please sign in to comment.