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: Make Refs Opt-in #6974

Closed
gaearon opened this issue Jun 6, 2016 · 24 comments
Closed

RFC: Make Refs Opt-in #6974

gaearon opened this issue Jun 6, 2016 · 24 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Jun 6, 2016

Note: This is my personal proposal.
Please don’t announce it anywhere as “React dropping refs!” 😄

Higher order components solve many problems of mixins, however they come with their own problems. The most painful one in my experience is that they hide the ref of the wrapped component, so they can’t be treated as transparent wrappers. This is well described in #4213.

As we prepare for de-emphasizing createClass and mixins I think it’s important that we treat higher order components as first class pattern in React, and provide full support for it without clashes with existing features. This means we need to fix refs to work well with higher order components.

In the community, I see that people embrace stateless functional components even though they don’t have public instances and don’t support refs pointed at them. I think that this is a good indication that refs are moving from being a commonly needed feature to an escape hatch, and so it can be further de-emphasized by becoming opt-in.

What Doesn’t Change

<div ref={...} /> works like before and provides node in a callback.
<StatelessFunctionalComponent ref={...} /> works like before and provides null in a callback.

Classes Opt Into Exposing a Ref

In the spirit of #4213 (comment), I propose that this.ref becomes an opt-in API on every class component. If you want your components to be “reffable” (that is, to expose their public instances as refs), you need to manually call it in your constructor:

class MyComponent extends Component {
  constructor(props) {
    super(props)
    this.ref(this) // I'm exposing my public instance!
  }

  ...
}

// Will print MyComponent instance
<MyComponent ref={instance => console.log(instance)} />

By Default, Don’t Expose Refs

If you don’t call this.ref(this) during the constructor, React will automatically call it with null:

class MyComponent extends Component {
  ...
}

// Will print null
<MyComponent ref={instance => console.log(instance)} />

This means that by default, class components will act just like functional components. There is no access to the instance unless the class opts in.

Automatic Cleanup

If the class opts in, it only needs to call this.ref in the constructor. React will take care of automatically calling it with null when the component unmounts.

New! Forwarding a Ref to Another Component

This is the new feature here.
Since we opt into refs, we can cleanly support ref forwarding for higher order components:

function wrap(WrappedComponent) {
  return class extends Component {
    ...

    render() {
      return <WrappedComponent ref={this.ref} />
    }
  }
}

By passing ref={this.ref}, we let WrappedComponent supply its own instance, if available. This way the fact that it’s wrapped with a higher order component becomes unobservable.

This also works fine if you conditionally switch between different components or delay rendering:

function wrap(WrappedComponent) {
  return class extends Component {
    ...

    render() {
      return this.state.isReady ? <Spinner /> : <WrappedComponent ref={this.ref} />
    }
  }
}

Let’s say isReady is false initially. React would take care of calling this.ref(null) after the constructor ran (since it knows the constructor never called this.ref(this)). So initially the parent receives null, as expected.

When WrappedComponent mounts, it will call this.ref(this) with its instance, which make it available to the parent. When WrappedComponent unmounts, React will call this.ref(null) for its instance, cleaning it up again.

The same works if we alternate between <WrappedComponent ref={this.ref} /> and <SomeOtherComponent ref={this.ref} />.

Upsides

Higher Order Components are Unobservable

This removes a common pain point in that wrapping a component with a HOC changes its public API.

Refs are Further Discouraged

By making them opt-in, we better signal that you shouldn’t use them for data flow. The component can also be certain that changing or removing an imperative method is not a breaking change because by default it doesn’t expose the ref. If it exposes the ref, this is done intentionally.

Providing Explicit Imperative APIs

The component may also choose to provide a subset of methods as its public API:

class MyComponent extends Component {
  constructor(props) {
    super(props)
    // I'm exposing just some stuff!
    this.ref({
      focus: this.focus.bind(this)
    })
  }

  focus() { ... }
  privateMethodIDontWantAnybodyToCall() { ... }
}

This lets the component choose which methods it wants to expose imperatively, and which are still considered implementation details.

Downsides

Migration Cost

This would be an easy enough codemod for most components (just add this.ref(this) to any class component). But it’s still a cost considering some of those components are on npm and out of your control. Arguably most third party components don’t provide imperative methods anyway, but this will cause some trouble.

Potential for Misuse

I can imagine people doing this.ref(this) and then <WrappedComponent ref={this.ref} /> in the render method. This could get confusing but I don’t see any easy way to prevent or warn about this.

Too Much Freedom

Technically you’d be able to pass this.ref(42), this.ref(findDOMNode(this)) or other weird things. Maybe we could limit possible values to React public instances and null. On the other hand, the ability to only provide a subset of methods as described in “Providing Explicit Imperative APIs” seems useful.

Other Considerations

this.props.ref?

We could have provided ref inside this.props as this.props.ref. I would argue that we don’t want to do this for two reasons:

  • We want React to still “manage” it partially by calling this.ref(null) when component unmounts. Otherwise it’s too easy to introduce memory leaks. Having magic behavior for one of the props would be unexpected.
  • We don’t want {...this.props} to transfer the ref over to the child as that would be unexpected in most cases.

What do you think?

cc @facebook/react-core

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 6, 2016

Bikeshed: this.setRef() would be more obvious and close to this.setState.

  constructor(props) {
    super(props)
    this.setRef(this) // I'm exposing my public instance!
  }

We could also make it less fun to type to signal discouragement:

  constructor(props) {
    super(props)
    this.setPublicInstance(this)
  }

Since this.setState/this.forceUpdate is not bound, it would be confusing that this.setPublicInstance is autobound:

// Can do that...
return <WrappedComponent ref={this.setPublicInstance} />

// ... but not that?
return <div onClick={this.forceUpdate}

We could either keep forwarding ref awkward:

return <WrappedComponent ref={this.setPublicInstance.bind(this)} />

Or bind setState/forceUpdate (wasteful in most cases).
Or live with the inconsistency.

@pierr
Copy link

pierr commented Jun 6, 2016

It would be great to push people to use more stateless function component and ease High Order Component use.

@poeschko
Copy link
Contributor

poeschko commented Jun 6, 2016

The ability "to provide a subset of methods as its public API" would be great! It would give a clear idea of what methods are available to consumers of the component. IMHO that's more important than to "limit possible values to React public instances and null" (which wouldn't allow specifying an object with a custom set of methods).

Does Flow understand the type of a ref? Could it be made to understand this new mechanism? (I haven't really tried or thought this through yet.)

@tkloht
Copy link

tkloht commented Jun 6, 2016

I think this could be split in two proposals:

  1. make it possible to set a ref, basically the this.ref() in the constructor part
    this would also make it possible to opt into NOT exposing a ref, or transferring a ref, or providing a subset as API.
  2. change the default behaviour into not exposing a ref

If I understand it correctly 1) should already make it possible to support ref forwarding for higher order components. The advantage is that the upgrade path would be better in my opinion if 1) would be implemented first, maybe with a warning in development that the default behaviour will change in the future. Then it would still be possible to change the default behaviour later.

Apart from that I really like the idea of exposing a subset of methods as public api, too.

@jimfb
Copy link
Contributor

jimfb commented Jun 6, 2016

Overall, I think this is a positive direction 👍.

However, I'd also like to move in the direction of ref not being special. Instead of this.ref, I'd prefer this.props.ref, and the component is expected to set it to null in componentWillUnmount.

This means an SFC can forward a ref by doing:

function MySFC(props) { return <Delegate ref={props.ref} /> }

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 6, 2016

However, I'd also like to move in the direction of ref not being special. Instead of this.ref, I'd prefer this.props.ref, and the component is expected to set it to null in componentWillUnmount.

  • I’m worried it’s too easy to forget to do it.
  • It’s never hard for us to do it so seems like we should do it.
  • It has the problem of {...this.props} carrying the ref over to the child which we usually don’t want.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jun 6, 2016

Is it true that only the second benefit is actually new compared to #4213 ? It seems like "Higher Order Components" are "Unobservable and Providing Explicit Imperative APIs" are also both covered by #4213. The only new thing here seems to be the idea that opt-in is more explanatory / less magic. Also, arguably more confusing before you understand what's going on.

I don't like a side-effectful API. Especially one that it used in the constructor since we don't know about the instance yet. It would have to mutate something on the instance that we read with a getter anyway.

I see how it is corresponding to this.setState but even that is not allowed in the constructor and something we explicitly want to get rid of, as the primary API, because of the confusing timing issues.

It might seem like we could invoke the ref callback right there, but the constructor is actually way too early and the callback needs to be able to be invoked at multiple times when the ref itself changes.

I assume the benefit over getPublicInstance is that it is easier to forward a ref because you don't need to store it on your own state.

@sebmarkbage
Copy link
Collaborator

Could you call this.ref(...) any time during your life-time to change the ref?

@glenjamin
Copy link
Contributor

I like the approach for forwarding refs, but for exposing a custom shape for your own ref I think a callback prop is better.

I ended up doing just that recently when I wanted something like a ref but with a slightly different lifecycle, eg. onRenderDone(node)

Perhaps something very specific would be suitable here this.forwardRef(otherComponentInstanceOrDOMNode).

@acdlite
Copy link
Collaborator

acdlite commented Jun 6, 2016

For whatever it's worth, whenever I've needed to work around refs and HOCs in my own apps, I've used a prop a la @jimfb's comment. It's pretty rare that I've needed to do this. It seems like the main benefit of keeping (class) refs as a first-class concept is because it's harder to screw up, but considering...

  1. class refs are an escape hatch that should generally be avoided
  2. React already expects you to do this for subscriptions (though maybe it shouldn't — a first-class API for subscriptions would be awesome)

...I think there's a strong argument for having people do it manually with a prop.

@acdlite
Copy link
Collaborator

acdlite commented Jun 6, 2016

I'm curious how common using class refs (as opposed to DOM refs) really is. I've done it maybe twice during my entire time as a React developer, and it both cases I was dealing with really old, debt-infested code.

Dan, you mentioned on Twitter that animations are a common use case, but I'm not sure quite what you mean. Does FB use them often?

@leoasis
Copy link

leoasis commented Jun 6, 2016

I see it is very common with form libraries. I try not to use that way though, but I can see how it may be useful probably for perf with big forms. Sometimes they are advertised directly in the examples: https://github.com/gcanti/tcomb-form

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 7, 2016

Is it true that only the second benefit is actually new compared to #4213 ? It seems like "Higher Order Components" are "Unobservable and Providing Explicit Imperative APIs" are also both covered by #4213. The only new thing here seems to be the idea that opt-in is more explanatory / less magic. Also, arguably more confusing before you understand what's going on.

I wasn’t sure if there’s any specific proposal in #4213. As you described in #4213 (comment), it’s hard to say when getPublicInstance() should get refired. With the proposal in this issue, setting a ref is imperative so we either fire in constructor or delegate to a child component to fire it. So yea, this is similar to ref={this.ref} proposal in that comment, except that we completely remove getPublicInstance() because these are two ways to achieve the same thing.

I assume the benefit over getPublicInstance is that it is easier to forward a ref because you don't need to store it on your own state.

Also that you have full control over when to refire.

Could you call this.ref(...) any time during your life-time to change the ref?

Yes. However in most cases you probably wouldn’t want to do it manually. There are two use cases, really:

I Want to Expose (a Subset of) My Instance

class TextField extends Component {
  constructor(props) {
    super(props)
    this.ref({
      focus: this.focus.bind(this)
    })
  }

  focus() { ... }
  privateMethodIDontWantAnybodyToCall() { ... }
}

I Want to Forward Ref (Which Could Be Refired)

class MyComponent extends Component {
  render() {
    return <TextField ref={this.ref} />
  }
}

class MyComponent2 extends Component {
  render() {
    return this.props.something ? <TextField ref={this.ref} /> : <MyComponent ref={this.ref} />
  }
}

@sebmarkbage
Copy link
Collaborator

The timing aspects are really difficult to get right. When you fire it in the constructor, you'll expose methods that are not yet ready to be used. Your component isn't fully initialized. It also doesn't yet have all its child refs.

That's why we ensured that your refs would be fired before componentDidMount so that you can use them in there. However, we also ensure that the ref that you expose get fired after componentDidMount so that you don't expose an incomplete instance that can't yet be used. Similarly updated refs get deferred to after reconciliation is done.

Otherwise, you move that burden to the handler of the ref to somehow know if they can call a method on you or not.

@sophiebits
Copy link
Collaborator

Also: The current contract is that whenever the value of .ref changes, the old function is called with null and the new one is called with the instance.

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 7, 2016

That's why we ensured that your refs would be fired before componentDidMount so that you can use them in there. However, we also ensure that the ref that you expose get fired after componentDidMount so that you don't expose an incomplete instance that can't yet be used. Similarly updated refs get deferred to after reconciliation is done.

This is super useful, thanks for explaining! I understand the constraints better now.

The current contract is that whenever the value of .ref changes, the old function is called with null and the new one is called with the instance.

Yea. If you just forward the ref with the proposal in this issue, I think this would be exactly what would happen, but there is definitely room for error if you start calling ref() imperatively by yourself.

@jeffbski
Copy link
Contributor

jeffbski commented Jun 10, 2016

I dislike refs myself, but currently it is the recommended way of obtaining the root component from render since render will no longer be synchronously returning the component. #6397

So if we are able to discourage use of refs, then we need an alternate way of getting the root component from render, maybe either:

  • a special ReactTestUtils render that does return a component synchronously OR
  • allow render to take a callback which will be called with the component once it is available

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 6, 2016

I understand refs a little better now.

@gaearon gaearon closed this as completed Jul 6, 2016
@tommoor
Copy link

tommoor commented Jul 18, 2016

@gaearon interested as to what solution you came to for HOC's - in my situation I'm wrapping an Input to provide additional functionality but would like to still be able to do this.refs.wrappedInput.value for example.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 18, 2016

@tommoor Are you asking about this proposal or workarounds in existing React versions?

@tommoor
Copy link

tommoor commented Jul 18, 2016

@gaearon sorry, I got the impression that you understood refs better and therefore this was no longer needed... looking through the other PR's/issues now perhaps this isn't actually the case? Is there a workaround that can seamless expose the wrapped components ref? 😄

@tommoor
Copy link

tommoor commented Aug 10, 2016

Still looking for a solution for this one, I tried using a callback ref but that seems to resolve to the HOC rather than it's child, I really expected that to work as well.

@brentvatne
Copy link
Contributor

@tommoor - one workaround I use is to add a prop to the component wrapped by your HOC to do something like this:

render() {
   <ComponentWrappedInHOC
     componentRef={component => { this._component = component; }}
   />
}

Inside ComponentWrappedInHOC

componentDidMount() {
  this.props.componentRef && this.props.componentRef(this);
}

componentDidUpdate() {
  this.props.componentRef && this.props.componentRef(this);
}

I also am curious about what @gaearon learned that led to closing this issue though :) Seemed like a good idea

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 15, 2018

This is addressed now by reactjs/rfcs#30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests