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 3 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.useRef((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
13 changes: 13 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,16 @@ 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 nextChildren = workInProgress.type.renderProp(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to validate before calling to get a nicer error message. See a similar check before context. I think Seb told me once it doesn’t affect perf because engine has to check it’s a function regardless and will likely remember that.

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 already validate in packages/react/src/useRef.js when the prop is passed initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

The validation in useRef.js is just a warning though, maybe use an invariant and throw early instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm maybe we should change context to work the same way then.

I don’t think we should make it ever invariant during creation. Makes inlining harder and will likely cause duplicate checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to rely on the warning to explain the error that would occur here when renderProp isn't a function? I understand not throwing during creation, but an explicit error seems better to me.

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'm not sure what consensus we've come to here. 😄

I think the warning during creation is good. Do we also want additional checks/invariants here? Or is it unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think warning during creation is a good idea, but I also think there should be an explicit error with a component stack trace when updateUseRef inevitably throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is 166d236 what you had in mind? The component stack will only be included in DEV mode.

This feels a little weird to me, checking both places.

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 +1113,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 useRef from './useRef';
import {
createElementWithValidation,
createFactoryWithValidation,
Expand All @@ -45,6 +46,7 @@ const React = {
PureComponent,

createContext,
useRef,

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
173 changes: 173 additions & 0 deletions packages/react/src/__tests__/useRef-test.internal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
/**
* 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('useRef', () => {
let React;
let ReactFeatureFlags;
let ReactNoop;

beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
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.useRef((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.useRef((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.useRef((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.useRef((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.useRef((props, ref) => null);

const ref = React.createRef();

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

it('should error if not provided a callback', () => {
expect(() => React.useRef(undefined)).toWarnDev(
'useRef requires a render prop but was given undefined.',
);
expect(() => React.useRef(null)).toWarnDev(
'useRef requires a render prop but was given null.',
);
expect(() => React.useRef('foo')).toWarnDev(
'useRef requires a render prop but was given string.',
);
});
});
25 changes: 25 additions & 0 deletions packages/react/src/useRef.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 useRef<Props, ElementType: React$ElementType>(
renderProp: (props: Props, ref: React$ElementRef<ElementType>) => React$Node,
) {
warning(
typeof renderProp === 'function',
'useRef requires a render prop but was given %s.',
Copy link
Contributor

Choose a reason for hiding this comment

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

"Render prop" in warning may confuse users. Maybe "render function"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's reasonable 👍

renderProp === null ? 'null' : typeof renderProp,
);

return {
$$typeof: REACT_USE_REF_TYPE,
renderProp,
};
}
3 changes: 3 additions & 0 deletions packages/shared/ReactSymbols.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ export const REACT_CONTEXT_TYPE = hasSymbol
export const REACT_ASYNC_MODE_TYPE = hasSymbol
? Symbol.for('react.async_mode')
: 0xeacf;
export const REACT_USE_REF_TYPE = hasSymbol
? Symbol.for('react.use_ref')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename these too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I kind of halted my renaming in the middle because of the ReactChildFiber suggestion I'm trying out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs rename

: 0xead0;

const MAYBE_ITERATOR_SYMBOL = typeof Symbol === 'function' && Symbol.iterator;
const FAUX_ITERATOR_SYMBOL = '@@iterator';
Expand Down
4 changes: 3 additions & 1 deletion packages/shared/ReactTypeOfWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ export type TypeOfWork =
| 10
| 11
| 12
| 13;
| 13
| 14;

export const IndeterminateComponent = 0; // Before we know whether it is functional or class
export const FunctionalComponent = 1;
Expand All @@ -37,3 +38,4 @@ export const Fragment = 10;
export const Mode = 11;
export const ContextConsumer = 12;
export const ContextProvider = 13;
export const UseRef = 14;