-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Changes from 3 commits
9bd0fdb
753dba7
56138d2
60efe32
166d236
0d565b5
508d52e
d5a196d
3583bf6
7c3d823
2661583
6185838
6d7d1ef
5321365
557cd79
04ec720
1b1deda
b91b3a3
c038d89
5b5b625
2a7b3c8
33bb141
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ import { | |
Mode, | ||
ContextProvider, | ||
ContextConsumer, | ||
UseRef, | ||
} from 'shared/ReactTypeOfWork'; | ||
import { | ||
PerformedWork, | ||
|
@@ -166,6 +167,16 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |
return workInProgress.child; | ||
} | ||
|
||
function updateUseRef(current, workInProgress) { | ||
const nextChildren = workInProgress.type.renderProp( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already validate in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The validation in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) { | ||
|
@@ -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, | ||
|
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.', | ||
); | ||
}); | ||
}); |
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.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Render prop" in warning may confuse users. Maybe "render function"? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's rename these too? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know! 😁