Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC #30: React.forwardRef implementation #12346

Merged
merged 22 commits into from
Mar 14, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/react-is/src/ReactIs.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
REACT_PORTAL_TYPE,
REACT_PROVIDER_TYPE,
REACT_STRICT_MODE_TYPE,
REACT_USE_REF_TYPE,
} from 'shared/ReactSymbols';

export function typeOf(object: any) {
Expand All @@ -38,6 +39,7 @@ export function typeOf(object: any) {
switch ($$typeofType) {
case REACT_CONTEXT_TYPE:
case REACT_PROVIDER_TYPE:
case REACT_USE_REF_TYPE:
return $$typeofType;
default:
return $$typeof;
Expand All @@ -58,6 +60,7 @@ export const Element = REACT_ELEMENT_TYPE;
export const Fragment = REACT_FRAGMENT_TYPE;
export const Portal = REACT_PORTAL_TYPE;
export const StrictMode = REACT_STRICT_MODE_TYPE;
export const UseRef = REACT_USE_REF_TYPE;

export function isAsyncMode(object: any) {
return typeOf(object) === REACT_ASYNC_MODE_TYPE;
Expand All @@ -84,3 +87,6 @@ export function isPortal(object: any) {
export function isStrictMode(object: any) {
return typeOf(object) === REACT_STRICT_MODE_TYPE;
}
export function isUseRef(object: any) {
return typeOf(object) === REACT_USE_REF_TYPE;
}
9 changes: 9 additions & 0 deletions packages/react-is/src/__tests__/ReactIs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,13 @@ describe('ReactIs', () => {
expect(ReactIs.isStrictMode(<React.unstable_AsyncMode />)).toBe(false);
expect(ReactIs.isStrictMode(<div />)).toBe(false);
});

it('should identify ref forwarding component', () => {
const RefForwardingComponent = React.forwardRef((props, ref) => null);
expect(ReactIs.typeOf(<RefForwardingComponent />)).toBe(ReactIs.UseRef);
expect(ReactIs.isUseRef(<RefForwardingComponent />)).toBe(true);
expect(ReactIs.isUseRef({type: ReactIs.StrictMode})).toBe(false);
expect(ReactIs.isUseRef(<React.unstable_AsyncMode />)).toBe(false);
expect(ReactIs.isUseRef(<div />)).toBe(false);
});
});
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
Mode,
ContextProvider,
ContextConsumer,
UseRef,
} from 'shared/ReactTypeOfWork';
import getComponentName from 'shared/getComponentName';

Expand All @@ -42,6 +43,7 @@ import {
REACT_PROVIDER_TYPE,
REACT_CONTEXT_TYPE,
REACT_ASYNC_MODE_TYPE,
REACT_USE_REF_TYPE,
} from 'shared/ReactSymbols';

let hasBadMapPolyfill;
Expand Down Expand Up @@ -357,6 +359,9 @@ export function createFiberFromElement(
// This is a consumer
fiberTag = ContextConsumer;
break;
case REACT_USE_REF_TYPE:
fiberTag = UseRef;
break;
default:
if (typeof type.tag === 'number') {
// Currently assumed to be a continuation and therefore is a
Expand Down
20 changes: 20 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
Mode,
ContextProvider,
ContextConsumer,
UseRef,
} from 'shared/ReactTypeOfWork';
import {
PerformedWork,
Expand Down Expand Up @@ -166,6 +167,23 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
return workInProgress.child;
}

function updateUseRef(current, workInProgress) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow why are these things so easy with Fiber

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know! 😁

const renderProp = workInProgress.type.renderProp;
invariant(
typeof renderProp === 'function',
'forwardRef requires a render function but was given %s.%s',
renderProp === null ? 'null' : typeof renderProp,
ReactDebugCurrentFiber.getCurrentFiberStackAddendum() || '',
);
const nextChildren = renderProp(
workInProgress.pendingProps,
workInProgress.ref,
);
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextChildren);
return workInProgress.child;
}

function updateMode(current, workInProgress) {
const nextChildren = workInProgress.pendingProps.children;
if (hasLegacyContextChanged()) {
Expand Down Expand Up @@ -1102,6 +1120,8 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
return updateFragment(current, workInProgress);
case Mode:
return updateMode(current, workInProgress);
case UseRef:
return updateUseRef(current, workInProgress);
case ContextProvider:
return updateContextProvider(
current,
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
ContextConsumer,
Fragment,
Mode,
UseRef,
} from 'shared/ReactTypeOfWork';
import {
Placement,
Expand Down Expand Up @@ -617,6 +618,8 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
return null;
case ContextConsumer:
return null;
case UseRef:
return null;
// Error cases
case IndeterminateComponent:
invariant(
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/React.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
isValidElement,
} from './ReactElement';
import {createContext} from './ReactContext';
import forwardRef from './forwardRef';
import {
createElementWithValidation,
createFactoryWithValidation,
Expand All @@ -45,6 +46,7 @@ const React = {
PureComponent,

createContext,
forwardRef,

Fragment: REACT_FRAGMENT_TYPE,
StrictMode: REACT_STRICT_MODE_TYPE,
Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
REACT_ASYNC_MODE_TYPE,
REACT_PROVIDER_TYPE,
REACT_CONTEXT_TYPE,
REACT_USE_REF_TYPE,
} from 'shared/ReactSymbols';
import checkPropTypes from 'prop-types/checkPropTypes';
import warning from 'fbjs/lib/warning';
Expand Down Expand Up @@ -297,7 +298,8 @@ export function createElementWithValidation(type, props, children) {
(typeof type === 'object' &&
type !== null &&
(type.$$typeof === REACT_PROVIDER_TYPE ||
type.$$typeof === REACT_CONTEXT_TYPE));
type.$$typeof === REACT_CONTEXT_TYPE ||
type.$$typeof === REACT_USE_REF_TYPE));

// We warn in this case but don't throw. We expect the element creation to
// succeed and there will likely be errors in render.
Expand Down
204 changes: 204 additions & 0 deletions packages/react/src/__tests__/forwardRef-test.internal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

describe('forwardRef', () => {
let React;
let ReactFeatureFlags;
let ReactNoop;

function normalizeCodeLocInfo(str) {
return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary for this test? If not you can remove the internal file name suffix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it, the number of yields vary between dev and prod :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Okay.

React = require('react');
ReactNoop = require('react-noop-renderer');
});

it('should work without a ref to be forwarded', () => {
class Child extends React.Component {
render() {
ReactNoop.yield(this.props.value);
return null;
}
}

function Wrapper(props) {
return <Child {...props} ref={props.forwardedRef} />;
}

const RefForwardingComponent = React.forwardRef((props, ref) => (
<Wrapper {...props} forwardedRef={ref} />
));

ReactNoop.render(<RefForwardingComponent value={123} />);
expect(ReactNoop.flush()).toEqual([123]);
});

it('should forward a ref to a chosen component', () => {
class Child extends React.Component {
render() {
ReactNoop.yield(this.props.value);
return null;
}
}

function Wrapper(props) {
return <Child {...props} ref={props.forwardedRef} />;
}

const RefForwardingComponent = React.forwardRef((props, ref) => (
<Wrapper {...props} forwardedRef={ref} />
));

const ref = React.createRef();

ReactNoop.render(<RefForwardingComponent ref={ref} value={123} />);
expect(ReactNoop.flush()).toEqual([123]);
expect(ref.value instanceof Child).toBe(true);
});

it('should maintain ref through updates', () => {
class Child extends React.Component {
render() {
ReactNoop.yield(this.props.value);
return null;
}
}

function Wrapper(props) {
return <Child {...props} ref={props.forwardedRef} />;
}

const RefForwardingComponent = React.forwardRef((props, ref) => (
<Wrapper {...props} forwardedRef={ref} />
));

let setRefCount = 0;
let ref;

const setRef = r => {
setRefCount++;
ref = r;
};

ReactNoop.render(<RefForwardingComponent ref={setRef} value={123} />);
expect(ReactNoop.flush()).toEqual([123]);
expect(ref instanceof Child).toBe(true);
expect(setRefCount).toBe(1);
ReactNoop.render(<RefForwardingComponent ref={setRef} value={456} />);
expect(ReactNoop.flush()).toEqual([456]);
expect(ref instanceof Child).toBe(true);
expect(setRefCount).toBe(1);
});

it('should not break lifecycle error handling', () => {
class ErrorBoundary extends React.Component {
state = {error: null};
componentDidCatch(error) {
ReactNoop.yield('ErrorBoundary.componentDidCatch');
this.setState({error});
}
render() {
if (this.state.error) {
ReactNoop.yield('ErrorBoundary.render: catch');
return null;
}
ReactNoop.yield('ErrorBoundary.render: try');
return this.props.children;
}
}

class BadRender extends React.Component {
render() {
ReactNoop.yield('BadRender throw');
throw new Error('oops!');
}
}

function Wrapper(props) {
ReactNoop.yield('Wrapper');
return <BadRender {...props} ref={props.forwardedRef} />;
}

const RefForwardingComponent = React.forwardRef((props, ref) => (
<Wrapper {...props} forwardedRef={ref} />
));

const ref = React.createRef();

ReactNoop.render(
<ErrorBoundary>
<RefForwardingComponent ref={ref} />
</ErrorBoundary>,
);
expect(ReactNoop.flush()).toEqual([
'ErrorBoundary.render: try',
'Wrapper',
'BadRender throw',
'ErrorBoundary.componentDidCatch',
'ErrorBoundary.render: catch',
]);
expect(ref.value).toBe(null);
});

it('should support rendering null', () => {
const RefForwardingComponent = React.forwardRef((props, ref) => null);

const ref = React.createRef();

ReactNoop.render(<RefForwardingComponent ref={ref} />);
ReactNoop.flush();
expect(ref.value).toBe(null);
});

it('should warn if not provided a callback during creation', () => {
expect(() => React.forwardRef(undefined)).toWarnDev(
'forwardRef requires a render function but was given undefined.',
);
expect(() => React.forwardRef(null)).toWarnDev(
'forwardRef requires a render function but was given null.',
);
expect(() => React.forwardRef('foo')).toWarnDev(
'forwardRef requires a render function but was given string.',
);
});

it('should error with a callstack if rendered without a function', () => {
let RefForwardingComponent;
expect(() => {
RefForwardingComponent = React.forwardRef();
}).toWarnDev(
'forwardRef requires a render function but was given undefined.',
);

ReactNoop.render(
<div>
<RefForwardingComponent />
</div>,
);

let caughtError;
try {
ReactNoop.flush();
} catch (error) {
caughtError = error;
}
expect(caughtError).toBeDefined();
expect(normalizeCodeLocInfo(caughtError.message)).toBe(
'forwardRef requires a render function but was given undefined.' +
(__DEV__ ? '\n in div (at **)' : ''),
);
});
});
25 changes: 25 additions & 0 deletions packages/react/src/forwardRef.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Copyright (c) 2014-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {REACT_USE_REF_TYPE} from 'shared/ReactSymbols';

import warning from 'fbjs/lib/warning';

export default function forwardRef<Props, ElementType: React$ElementType>(
renderProp: (props: Props, ref: React$ElementRef<ElementType>) => React$Node,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really a render prop since it's not passed at render time nor a prop. It's more like, just renderFn or render maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I like renderFn

) {
warning(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (__DEV__) around it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning is an empty function if not DEV, so I didn't think that was necessary. But sure!

typeof renderProp === 'function',
'forwardRef requires a render function but was given %s.',
renderProp === null ? 'null' : typeof renderProp,
);

return {
$$typeof: REACT_USE_REF_TYPE,
renderProp,
};
}
Loading