-
Notifications
You must be signed in to change notification settings - Fork 558
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
ref-forwarding API #30
Conversation
I really like this approach. Makes a lot of sense for existing applications and should be something fairly simple to build into the compiler to handle too. |
Maybe this is out of scope, but could be worth mentioning that this same strategy can be used for all HOCs. For example: function ThemedButton(props, theme) {
return <button className={'theme-' + theme} {...props} />
}
export default withTheme(ThemedButton); where |
Also, all the new proposed APIs: For example, the new context API is a FaCC factory: const {Consumer} = React.createContext('theme');
<Consumer>{theme => <App />}</Consumer> But if the context API would follow the pattern proposed in this RFC, it would look something like this: const {withContext} = React.createContext('theme');
withContext(theme => <App />); Does the <React.Ref>{(props, ref) =>
<LegacyRelayContextConsumer {...props} forwardedRef={ref} />
}</React.Ref> Or would |
@streamich This is an HOC, except it only works for functional components. The difference is that instead of injecting additional props, it passes additional arguments. The larger idea behind this proposal is that we could use the same pattern for other APIs, too. Like, say, context: const ThemeContext = React.createContext('light');
function ThemedButton(props, theme) {
return <button className={'theme-' + theme} {...props} />;
}
export default ThemeContext.withContext(ThemedButton); |
@sebmarkbage suggested we use the function Counter(props, count, updateCount) {
return (
<>
Count: {count}
<button onClick={() => updateCount(c => c + 1)}>+</button>
</>
);
}
export default useState(Counter, 0); Or no arguments at all: function ComputationallyExpensiveComponent(props) {
const thing = somethingThatTakesALongTime(props);
return <Whatever thing={thing} />;
}
const MemoizedComponent = usePure(ComputationallyExpensiveComponent); |
Thanks @trueadm!
Sorry, I misread this message initially. I still agree with the sentiment of your comment, but I think mentioning it might be a bit out of scope for this RFC. 😄
@sebmarkbage always has good ideas. 😄I'll update! |
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.
This would work 100% for styled-components. innerRef
is one of the uglier patches of our API and I'd love to get rid of it and make the wrapper components truly transparent and pass-through.
/cc @kitten @probablyup
``` | ||
|
||
Unfortunately, within the current limitations of React, there is no (transparent) way for a | ||
a `ref` to be attached to the above `ThemedComponent`. Common workarounds typically use a special prop (e.g. `componentRef`) like so: |
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.
We use innerRef
for styled-components so users can get access to the underlying DOM node, see https://www.styled-components.com/docs/advanced#refs
How does this handle static class properties? It would seem to swallow them, if you did something like: class DataContainer extends Component {
static fragments = {widget: gql`...`}
}
export default React.useRef((props, ref) => (
<DataContainer {...props} forwardedRef={ref} />
)); Perhaps the solution / challenge is the same as other higher-order components, in that it would either require buy-in on the original component itself so that it attached static properties to the wrapper or would require a workaround like That said, it would be nice if however this worked did not require knowledge of static properties, especially in the case where you want to wrap a third-party component with This very well may be a non-issue. |
That's great to hear, Max!
I don't think static properties are a problem here (but please correct me if I'm overlooking something). I think your observation about attaching/forwarding them to the exported ref-type is a reasonable solution. The key part is that you would be able to do this in a way that's transparent to users of the thing you'r wrapping. (That isn't currently possible, due to refs.) |
I'd be wary of this at the library level. When I write a HOC I actually don't use So if we create something at the library level that shadows static properties and the library has to workaround it, I'm worried that if no-one remembers to take them into account we'll end up with something baked into the library (where we can't just switch to a different implementation) that handles statics but forgets about instance methods on the prototype. |
Thanks for writing this! I'm having a similar issue where I have some HOCs over an and need to expose it's That's also why I wrote https://github.com/elado/hoist-non-react-methods Unrelated to this RFC so hiding this to reduce noiseNot really related, but if there's any API change suggested, could React specific props be more distinct?e.g. <Comp $ref={...} $key={...} />
<Comp react-ref={...} react-key={...} />
<Comp react.ref={...} react.key={...} /> this will allow more explicit way to define react props and prevent breaking changes when new react-specific props are added. Also will allow proper spread of objects that may contain |
@elado XHTML does have a concept of namespaces, could do something like |
Namespaces in JSX have been discussed before: facebook/jsx#81 We would love to have them for styled-components so we can remove the tag whitelist but it doesn't seem like that's happening. |
I might be misunderstanding your concern, but this ref-forwarding proposal is specifically intended to help with the shadowing of instance methods like
I don't think static properties is a common concern. Maybe I'm wrong. But if this is a case for a given component, there is a workaround (discussed above). WRT namespaced attributes, that's outside of the scope of this RFC. 😄 |
True, I could just forward the ref to the top-level component. But I am worried that: A) sometimes I want to forward refs for something deep but I still want to offer direct methods on my component for the top level component (Say a |
This comment doesn't make sense to me. I think the In the case you describe, the top-level component sounds like it would be the class PromptDialog extends React.Component {
inputRef = React.createRef();
show() {
// Show...
}
hide() {
// Hide...
}
focus() {
if (this.inputRef.value != null) {
this.inputRef.value.focus();
}
}
render() {
return isShown ? <input ref={this.inputRef} /> : null;
}
} External users of your dialog would use it like so: class Example extends React.Component {
dialogRef = React.createRef();
componentDidMount() {
this.dialogRef.value.show();
this.dialogRef.value.focus();
}
render() {
return <PromptDialog ref={dialogRef} />;
}
} It doesn't make sense for a single class PromptDialog extends React.Component {
show() {
// Show...
}
hide() {
// Hide...
}
render() {
// Forward props.inputRef so it points to the inner <input>
return isShown ? <input ref={this.props.inputRef} /> : null;
}
}
function wrapForSomeReason(Component) {
function WrapperComponent(props) {
// (This wrapper intentionally left blank to simplify the example.)
return <Component {...props} ref={props.forwardedRef} />;
}
// Forward top-level `ref` so it points to Component
return React.useRef((props, ref) => (
<WrapperComponent {...props} forwardedRef={ref} />
));
}
// Top-level `ref` will point to PromptDialog
const WrappedDialog = wrapForSomeReason(PromptDialog);
I don't really understand this comment, which makes me wonder if you're thinking of this proposal as something fundamentally different than I am. (Or maybe I'm just...misunderstanding your comment.) 😄 Do my above examples clarify any? If not, could you write a small example component that illustrates your point? I don't see how this proposal complicates or prevents you from doing what you're describing. |
@bvaughn Sorry, I read the RFC but I'll admit I found it hard to understand. Your last example makes sense and fits. I was just going off another comment talking about the interactions with hoisting and I worried because HOC usage is fairly common for me. For |
No problem. If you have suggestions for ways I can clarify the RFC wording, let me know. This RFC is intended to help with the specific case you're describing - so that you can wrap your |
Forgive me if this has already been covered — did a quick read-through, and didn't see anything. I just want to confirm that the For reference, type RefObject<T> = {
value: T | null
}; In some cases, its necessary for a wrapped component to both forward a ref and access it itself. I'm looking for assurance that the following code would work: class FancyTextbox extends Component {
state = {
value: '',
};
// In this contrived method, I'd like to be able to access
// the same <input> ref that I'm forwarding.
reset() {
this.setState({ value: '' }, () => {
if (this.props.forwardedRef.value) {
this.props.forwardedRef.value.focus();
}
});
}
render() {
return (
<div>
<input
type="text"
ref={this.props.forwardedRef}
value={this.state.value}
onChange={(value) => this.setState({ value })}
/>
</div>
);
}
}
export default React.useRef((props, ref) => (
<FancyTextbox {...props} forwardedRef={ref} />
)); The above example is exceedingly contrived, but the actual usage I have in mind concerns binding native |
No. The In your above example, you would have to do something (admittedly a bit awkward) like this: class FancyTextbox extends Component {
state = {
value: ""
};
reset() {
this.setState({ value: "" }, () => {
if (this.ref) {
this.ref.focus();
}
});
}
render() {
return (
<div>
<input ref={setRef} />
</div>
);
}
setRef = ref => {
this.ref = ref;
const { forwardedRef } = this.props;
if (typeof forwardedRef === "function") {
forwardedRef(ref);
} else {
forwardedRef.current = ref;
}
};
} |
Why is setting How resistant would you be to normalizing the type of If this API is borne of a wish to simplify and normalize nested ref handling, it'd be a shame if it ended up trading old boilerplate for new (i.e., trading manual, proprietary ref unwrapping with the code above, which will need to be aware of all possibly-legal ref values, and is thus not future proof). I feel like this RFC + the |
Forcing refs to be a Rather than forcing a single type of ref, if it's important for user code be able to essentially set a ref, we should expose a helper for that instead of restricting to a single type. i.e. A |
I'm perfectly fine with @dantman's approach, too. I missed the boat on the Regarding the use cases served by a callback class Wrapper extends Component {
lastRef = this.props.wrappedRef.value;
componentDidUpdate() {
const { lastRef } = this;
const { value: nextRef } = this.props.wrappedRef.value;
if (lastRef !== nextRef) {
// Do whatever you would've done in your callback ref.
this.lastRef = nextRef;
}
}
// ...
}
// ref is of type RefObject<T>
export default React.useRef((props, ref) => (
<Wrapper {...props} forwardedRef={ref} />
)); |
The updated example by @jamesreggio does not handle the callback ref case, so it would not work. Setting that aside, using the property initializer case for extracting a |
I agree it would be nice to have a built-in bulletproof to compose refs (the result would always have to be a functional ref). I don’t feel like it has to be a part of this proposal though. It is also not a fundamentally new capability so perhaps it could live outside of React (I know at least one library that does this although it’ll need to be updated for object refs.) Maybe a separate RFC? |
The point of this example was to show how to attain the flexibility of a callback ref if
Name change aside, it sounds like you're saying that it's safe and supported to write to the I'm fine punting the normalization concerns to user-space or a separate RFC, but in order to do so, I want to explicitly confirm what has been implicit to this conversation. Specifically: The This guarantee means that we can assume that the value of (I feel like there may be certain internal implementation conveniences or optimizations that could be leveraged if the |
Yes. (When you want to manage it manually, such as the case when composing refs.)
Yes. (Intentionally because always “normalizing” it to some form might be an unnecessary cost in most cases.) |
As I think about the use-case you raise, @jamesreggio, I find myself warming up to the idea of a new package that would help compose both ref-types. I'll add it to my list of things to write an RFC for (unless you beat me to it). Edit I can't promise this means that we'll actually release this, just that we'll give it consideration. 😄 |
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.
Typo
text/0000-ref-forwarding.md
Outdated
|
||
I recently began contributing to [`react-relay` modern](https://github.com/facebook/relay/tree/master/packages/react-relay/modern) in order to prepare it for the [upcoming React async-rendering feature](https://reactjs.org/blog/2018/03/01/sneak-peek-beyond-react-16.html). | ||
|
||
One of the first challenges I encountered was that `react-relay` depends on the legacy, unstable context API, and values passed through legacy `context` aren't accessible in the new, [static `getDerivedStateFromProps` lifecycle](https://github.com/reactjs/rfcs/blob/master/text/0006-static-lifecycle-methods.md#static-getderivedstatefrompropsnextprops-props-prevstate-state-shapestate--null). The long-term plan for `react-relay` is to use the [new and improved contex API](https://github.com/reactjs/rfcs/blob/master/text/0002-new-version-of-context.md) but this can't be done without dropping support for older versions of React- (something `react-relay` may not be able to do yet). |
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.
new and improved contex
API -> new and improved context
API
text/0000-ref-forwarding.md
Outdated
|
||
1. Add a new class method, e.g. `getPublicInstance` or `getWrappedInstance` that could be used to get the inner ref. (Some drawbacks listed [here](https://github.com/facebook/react/issues/4213#issuecomment-115019321).) | ||
|
||
2. Specify a ["high-level" flag on the component](https://github.com/facebook/react/issues/4213#issuecomment-115048260) that instructs React to forward refs past it. This appraoch could enable refs to be forwarded one level (to the immediate child) but would not enable forwarding to deeper child, e.g.: |
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.
appraoch
-> approach
text/0000-ref-forwarding.md
Outdated
} | ||
``` | ||
|
||
3. [Automatically forward refs for stateless functions components](https://github.com/facebook/react/issues/4213#issuecomment-115051991). (React currently warns if you try attaching a `ref` to a functional component, since there is no backing instance to reference.) This appraoch would not enable class components to forward refs, and so would not be sufficient, since wrapper components often require class lifecycles. It would also have the same child-depth limitations as the above option. |
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.
appraoch -> approach
@bvaughn Yes that is it exactly! I will definitely allign that doc in the near future. |
I don't know if there are plans on making ref forwarding a part of React yet, but I just wrote a quick library that allows hoisting instance methods from inner refs. It uses context (old API), and some JS magic. First draft here: |
That's what this RFC was for 😄and the feature is part of the upcoming 16.3 (currently available as an alpha on NPM). |
I think the final comment period is done for this PR. |
@bvaughn @gaearon there's one use case I'm still not sure how class Field extends Component {
focusInput() {
this.input.focus()
}
render() {
return (
<div>
<input ref={i => (this.input = i)} />
</div>
)
}
}
class App extends Component {
render() {
return (
<div>
<Field ref={i => (this.field = i)} />
<button onClick={() => this.field.focusInput()}>Focus</button>
</div>
)
}
} when class Field extends Component {
focusInput() {
this.input.focus()
}
render() {
return (
<div>
<label>{this.props.label}</label>
<input ref={i => (this.input = i)} />
</div>
)
}
}
// one or more HOCs from my code/other libraries
const WrappedField = someHOC(Field)
class App extends Component {
render() {
return (
<div>
<WrappedField ref={i => (this.field = i)} />
{/* this will fail */}
<button onClick={() => this.field.focusInput()}>Focus</button>
</div>
)
}
} In Or even with Redux |
This is meant for library developers writing the HOC, not for consumers. But you could write your own HOC which renames forwards the ref as some prop (say |
@satya164 is right. Worst case, assuming // Extracts a forwared ref-prop and applies as a ref
function applyForwardedRefTo(Component) {
return function ApplyForwardedRef({ forwardedRef, ...props }) {
return <Component {...props} ref={forwardedRef} />;
};
}
// Intercepts an incoming ref and forwards it as a prop
function forwardRefTo(Component) {
function ForwardRefTo(props, ref) {
return <Component {...props} forwardedRef={ref} />;
}
return React.forwardRef(ForwardRefTo);
}
const WrappedField = forwardRefTo(someHOC(applyForwardedRefTo(Field))); Here's a CodeSandbox example: |
This means that current non-maintained libraries will probably never get this feature Instead of waiting for updates/submitting PRs, I think there should be a global, bulletproof solution. That's why I came up with https://github.com/elado/react-ref-method-forwarder in the first place.
import React, { Component } from 'react'
// just a 3rd party library that exports a HOC
import dimensions from 'react-dimensions'
class Field extends Component {
focusInput() {
this.input.focus()
}
render() {
return (
<div>
<input ref={i => (this.input = i)} />
</div>
)
}
}
const forwardRefOuter = Wrapped => {
return React.forwardRef((props, ref) => <Wrapped {...props} innerRef={ref} />)
}
// this *WORKS*, but throws a warning:
// Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail.
// const forwardRefInner = Wrapped => props => <Wrapped {...props} ref={props.innerRef} />
const forwardRefInner = Wrapped => {
return class extends Component {
render() {
return <Wrapped {...this.props} ref={this.props.innerRef} />
}
}
}
export default forwardRefOuter(dimensions()(forwardRefInner(Field)))
/* or
@forwardRefOuter
@dimensions()
@forwardRefInner
class Field...
*/
// App.js - normal ref usage
class App extends Component {
render() {
return (
<div>
<Field ref={i => (this.field = i)} />
<button onClick={() => this.field.focusInput()}>Focus</button>
</div>
)
}
} Edit: Snap 😄 @bvaughn beat me me to it ;) |
This goes for any RFC. We’ve always been improving the way React apps and libraries are written in the future rather than adding retroactive hacks that get around an abstraction. If something is unmaintained it will likely bump into breaking changes between majors in any case. I understand it’s not always convenient, but it’s a larger discussion and unrelated to this particular API. |
View formatted RFC
Implementation at facebook/react/pull/12346