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

Implement Better Refs API #1373

Closed
sebmarkbage opened this Issue Apr 8, 2014 · 30 comments

Comments

Projects
None yet
3 participants
@sebmarkbage
Member

sebmarkbage commented Apr 8, 2014

The ref API is broken is several aspects.

  • You have to refer to this.refs['myname'] as strings to be Closure Compiler Advanced Mode compatible.
  • It doesn't allow the notion of multiple owners of a single instance.
  • Magical dynamic strings potentially break optimizations in VMs.
  • It needs to be always consistent, because it's synchronously resolved. This means that asynchronous batching of rendering introduces potential bugs.
  • We currently have a hook to get sibling refs so that you can have one component refer to it's sibling as a context reference. This only works one level. This breaks the ability to wrap one of those in an encapsulation.
  • It can't be statically typed. You have to cast it at any use in languages like TypeScript.
  • There's no way to attach the ref to the correct "owner" in a callback invoked by a child. <Child renderer={index => <div ref="test">{index}</div>} /> -- this ref will be attached where the callback is issued, not in the current owner.

I think that the solution must ultimately be some kind of first class ref that can be passed around. These refs can be chained to create multi-owner refs very efficiently. By creating this object for the ref, we can also get rid of keeping track of owners on descriptors. Saving perf for the common idiomatic case of not using refs.
A secondary goal, which may or may not be as important is the idea of making the resolution of refs asynchronous so that you can respond after a batched flush/reconciliation.

The concept of a first class ref is basically a reference to an object that doesn't exist yet. Luckily there's a first class notion of this in the language already... It's called a Promise.
You create a new Ref instance which is just Promise object that will eventually resolve to the actual instance.

class Foo {

  myDivRef = React.createRef();

  handleTick() {
    this.myDivRef.then(myDivNode => {
      this.setState({ width: myDivNode.offsetWidth });
    });
  }

  render() {
    return (
      <C tick={this.handleTick}>
        <div ref={this.myDivRef} />
        <CustomComponent context={this.myDivRef} />
      </C>
    );
  }

}

Since this builds on top of Promises we would be able to get async/await language features that allow us to do something like this:

  async handleTick() {
    this.setState({ width: (await this.myDivRef).offsetWidth });
  }

This solves all those use cases AFAIK. The asynchronous API is a little difficult to deal with. But it makes it less weird than the alternative when batching is involved.

An unsolved problem is that refs can update to point to a different instance. In that case the Promise would need to be re-resolved. This is why Promises are not good enough and we ultimately need something like an Observable that can handle multiple values. We can't wait for that spec though. Maybe we just allow our promises to be reset and if you call then(...) again, you get a new value?

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

On top of this we could build a dynamic RefMap that lazily creates ref promises. This allows easy creation of refs for sets of data. It also makes a nicer upgrade path for existing code that assumes that these are just strings:

class Foo {
  refs = new React.RefMap();

  handleTick() {
    this.refs.get('myDiv').then(myDivNode => {
      this.setState({ width: myDivNode.offsetWidth });
    });
  }

  render() {
    return (
      <C tick={this.handleTick}>
        <div ref={this.refs.get('myDiv')} />
        <CustomComponent context={this.refs.get('myDiv')} />
      </C>
    );
  }
}

We also probably need to provide a synchronous API as an upgrade path:

  handleTick() {
    var myDivNode = this.myDivRef.doExpensiveWorkRightNowAndLetMeHaveMyNodeNow();
    this.setState({ width: myDivNode.offsetWidth });
  }

Similarly we need this for ref maps.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 8, 2014

This makes a lot of sense to me, though I guess I'm not totally convinced of the need for it to be async. I'd expect most uses to be in mount-ready handlers though I suppose it's possible for a DOM event handler to be called when an update is pending.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 8, 2014

Do you have a plan for what to do with descriptors that are mounted in more than one place?

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

Descriptors that are mounted in more than one place should not have refs I guess. Warning maybe?

Not only DOM events but we want to batch timers and data fetching events too. The goal being that flush only happens on rAF. Therefore any callback could have the refs pending.

Additionally the behavior of didMount and didUpdate handlers are currently undefined in regards to when they fire in relation to their children and therefore refs. For example componentDidUpdate is not guaranteed to fire after the children has fully mounted.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

Actually in the current state, since they're queued on the DOM generation queue, I guess they're guaranteed right now. We may be able to preserve this behavior. Not sure.

An alternative API would be to force refs to be extracted in just those two lifecycle hooks. Then you can store them where ever. That might lead to memory leaks.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

On a second thought, didMount/didUpdate is not enough to keep ref handles up-to-date. Since a child can choose to unmount/remount that descriptor at any point. Then those events are not enough. Potentially the ref life-cycle callback could be connected to the owner instance but that's not as flexible as a first class ref I guess.

However, if you want something like a resize handle when a child is actually mounted/remounted it might be troublesome to set up a component-level subscription on the child. I.e. you have to call .then at some point before that. However, maybe that should be handled with a more explicit callback?

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 8, 2014

Does this mean that if a child never mounts its argument, the ref will "hang" and never resolve? That sounds odd to me.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

Yea...

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

I suppose the promise should be rejected if the next flush doesn't lead the ref to be resolved.

  handleTick() {
    this.refs.get('myDiv').then(myDivNode => {
      this.setState({ width: myDivNode.offsetWidth });
    }, error => {
      this.setState({ width: 0 });
    });
  }
@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 8, 2014

Presumably the same thing should happen if a ref isn't used at all? I guess that would make a reasonable API, but unless I'm missing something, each ref object won't know which component it belongs to (alternatively, a component won't have a list of all of its refs) and thus can't know when to mark itself as rejected.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 8, 2014

Every ref object can go from resolved to unresolved and back. We track that in the same way as attach/detach ref. E.g. props.ref.attach(this); or if it's unmounted: props.ref.detach(this);

The act of calling ref.then(...) can register that ref as "pending". After the next flush is done, all the pending refs gets their callback invokes.

for (let ref of pendingRefs) {
  if (ref.hasAttachedInstance) ref.resolve(); else ref.reject();
}
@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 15, 2014

With this API you wrote out, there's no way to get a component instance, only a DOM node; this means you can't call methods, etc. on child components. Intentional?

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 16, 2014

Actually I had imagine that a component instance of a ReactDOMComponent could become the DOM node. This is still controversial though. The alternative is just an empty object with a getDOMNode method on it.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 16, 2014

Well sometimes you want a ref to a composite, right? I haven't thought about it much but that sounds like an odd plan to me. The uniform .getDOMNode() across all types of browser components right now is pretty nice.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 16, 2014

Yes, a ref to a composite would be a still be the composite instance, unless it's a stateless component which would not be allowed to have a ref or resolve to null.

.getDOMNode() is a problematic API since it leaks internals of a component and drills down through multiple levels of abstractions without them explicitly allowing that.

It is expected to return a single node but what if your composite is returning a fragment of multiple nodes?

It also needs to go on the base class of every composite, even ART composites, render tree composites, MarkDown composites or whatever. Unless every component that wants it is required to opt-in to a special DOM base class. That doesn't guarantee that your ref has the method though.

I think a much better API would be React.getDOMNodeFrom(instance) which would work even if the instance is a real DOM node (it would just return itself).

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 16, 2014

In any case, we need to figure out what to do with these wrapper components -- if DOM node instances diverge from composites, it's going to be odd when you write <input ref={this.myInput} /> expecting a DOM node and you get a composite in return.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 16, 2014

Yea. I wonder if it can be a pass-through? So that ReactDOMInput does <RealInput ref={this.props.ref} />. That doesn't really make sense but something like that would be cool.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Apr 18, 2014

An alternative idea... Make descriptors into ref-promises.

  render() {
    var foo = <Foo />;
    return <div onClick={() => foo.then(inst => inst.doX())}>{foo}</div>;
  }

This also provides a nice reset functionality if the ref is ever swapped out.

  handleClick() {
    this.foo.then(inst => inst.doX());
  }

  render() {
    this.foo = <Foo />; // ugh side-effect in render (puke)
    return <div onClick={this.handleClick}>{this.foo}</div>;
  }

This also works nice with multiple owners.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Nov 20, 2014

This is also inherently an imperative API because it exposes an imperative handle. Even if it's just an ID or handle. We also already effective allow people to store ephemeral state on classes.

An author should be prepared that a component class can be destroyed and restored at any given point. E.g. if we want to "hibernate" state and through away the class instance. This still allows it.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 23, 2015

@sebmarkbage When updating, do refs get called on every rerender? (Or maybe even if shouldComponentUpdate returns false?)

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Jan 23, 2015

no, just if it changes.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 23, 2015

So if I render <Foo /> then render <Foo ref={(f) => this.foo = c} /> on top of it, the ref is never called?

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Jan 23, 2015

No that would be a change in the ref. Just like if you change the ref name. So it should be called.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 23, 2015

In conventional usage though it'll be a different function each time, yeah?

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Jan 23, 2015

Good point. Didn't think about that. Maybe just fire if it goes from null to function?

@sebmarkbage sebmarkbage referenced this issue Jan 23, 2015

Closed

Umbrella 0.13 #2809

14 of 23 tasks complete

sophiebits added a commit to sophiebits/react that referenced this issue Jan 23, 2015

stryju added a commit to stryju/nextgram that referenced this issue Oct 26, 2016

Update modal.js to use ref callbacks instead of strings
using string `ref`s can be dangerous, so it's safer to use callback form :)

facebook/react#1373

impronunciable added a commit to now-examples/nextgram that referenced this issue Nov 2, 2016

Update modal.js to use ref callbacks instead of strings (#3)
using string `ref`s can be dangerous, so it's safer to use callback form :)

facebook/react#1373

@MSYuey MSYuey referenced this issue Aug 21, 2018

Merged

Replace legacy string refs with callback refs #344

3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment