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

Forward refs / Support getPublicInstance #4213

Closed
cpojer opened this issue Jun 24, 2015 · 25 comments
Closed

Forward refs / Support getPublicInstance #4213

cpojer opened this issue Jun 24, 2015 · 25 comments

Comments

@cpojer
Copy link
Member

@cpojer cpojer commented Jun 24, 2015

For 0.14 we'd like a feature that allows us to forward refs or define what the public instance of a component should be. That way we can make higher order components that are completely transparent.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Jun 24, 2015

@sebmarkbage I told @cpojer this probably wouldn't be hard to implement if we can agree on the semantics. Does findDOMNode (etc) work on the ref you get if you implement getPublicInstance? If not, this might be really easy.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Jun 24, 2015

Rant: Yea I've been thinking about this. This feature smells really wrong to me, since the whole point of moving refs off props was to make them an outside-concept that the framework controlled. It seems like if you want to control it, we should just make them props. That way you have the ability to fully control them and the timing. However, you don't always want to have to resolve them yourself. Also, you don't want to accidentally transfer a ref by using the property spread. Although if refs finally become less common, they should probably become manually invoked functions. So I'm torn.

Non-rant: I'm not sure the getPublicInstance API is going to be sufficient for other use cases. It doesn't provide the capability to refire.

class Container {
  state = { toggle: false };
  tick = () => {
    this.setState({ toggle: !this.state.toggle });
  }
  componentDidMount() {
    setInterval(this.tick, 100);
  }
  render() {
    return <div>{this.state.toggle ? this.props.children : <Pause />}</div>;
  }
}

class Wrapper {
  getPublicInstance() {
    return this.refs.myComponent;
  }
  render() {
    return <Container><Component ref="myComponent" /></Container>;
  }
}

In this example, there is nothing that binds the unmount/remount scenario to the Wrapper. We can't recall getPublicInstance to get the fresh component. We will keep having a stale version.

Something like this might be better:

class Wrapper {
  render() {
    return <Container><Component ref={this.ref} /></Container>;
  }
}

However, how do we know that a ref was transferred? Maybe if the function fires before this component's didMount, but that is very subtle and breaks down for the case above where it doesn't fire until later.

Maybe we add something like getControlledRef but that's an extra API and would be an side-effectful which is not ideal.

Suggestions?

@jimfb
Copy link
Contributor

@jimfb jimfb commented Jun 24, 2015

Personally, I'm a fan of refs becoming manually invoked functions, and so that probably influences my thinking/solution...

Make component manually invoke the ref, wrappers can forward the ref. For now, call it myref to avoid the autoinvoking that React does. Then the wrapper becomes:

class Wrapper {
  render() {
    return <Container><Component myref={this.props.myref} /></Container>;
  }
}

It's not "completely transparent", since the base component must expose a manually invoked ref instead of relying on the one provided automatically by React, but assuming you're in control of the base component, this works, right? Or maybe I'm totally miss-understanding the request :P.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Jun 24, 2015

The request is to provide a seamless API that is natural to use with React so that HoC can be unobservable.

@jimfb
Copy link
Contributor

@jimfb jimfb commented Jun 24, 2015

If we care about it being unobservable: Could we have a flag on the class that says "I'm a high level component, don't resolve my ref, I'll manually invoke it and/or pass it to someone else"? That would make the high level components completely transparent and also enable a clean migration path for making all refs manually invoked (if we decide to do that someday; migration path is that every component that takes in a ref must have that flag set).

So the new code looks like:

class Wrapper {
  render() {
    return <Container><Component ref={this.props.ref} /></Container>;
  }
}
Wrapper.manualRefs = true;
@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Jun 25, 2015

We already use static flags for things like context types etc. We might want to use flag to indicate that a component's state is pure too. That seems natural. Could potentially work for stateless functions too.

However, that brings up an interesting point. Should stateless functions automatically transfer the ref since they don't have instances?

@jimfb
Copy link
Contributor

@jimfb jimfb commented Jun 25, 2015

My vote would be to have refs be illegal on stateless functions, since there is no component for them to reference. That allows us to punt on the decision until a future date. It also gives us a reasonable opportunity to warn if someone tries to attach a ref to a stateless function (since, in all likelihood, the implementor of the stateless function will have forgotten to properly forward/handle the ref, which will lead to user confusion unless we can reject at the time the ref is attached thereby sidestepping the issue).

Making refs illegal on stateless functions won't result in lost functionality, since a user can always regain any potentially lost functionality by wrapping the stateless function in a HOC, thereby regaining the functionality including the ability to attach a ref and use findDOMNode.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Jun 25, 2015

My vote would be to have refs be illegal on stateless functions, since there is no component for them to reference.

Seems like a rather arbitrary restriction to me. findDOMNode is as useful on a stateless function as it is on any other component. (I'm fine with introducing a way to restrict use of findDOMNode but this seems unrelated.)

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Jun 25, 2015

The use case for stateless functions is basically to A) provide syntax sugar for another component and/or B) lock down the API by excluding configuration of some props.

For A it would be a nice convenience to forward the ref, but for B it would be detrimental to locking down the API. So I don't know. In cases like that I tend to error on the side of the easiest to change from, which is the more restrictive form - i.e. making refs not work and possibly warn on these.

@jimfb
Copy link
Contributor

@jimfb jimfb commented Jun 25, 2015

👍 Sounds like we've got a game plan here.

@gaearon
Copy link
Member

@gaearon gaearon commented Oct 12, 2015

Just bumped into this a few days ago. React now warns when we put a ref onto a stateless component, but I do it from HOC because the user might want to have a ref: #4943 (comment)

I'm thinking of introducing a prop like wrappedRef to my HOC and doing something like return <WrappedComponent ref={this.props.wrappedRef}.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Oct 12, 2015

...or just use a class for your HoC.

@gaearon
Copy link
Member

@gaearon gaearon commented Oct 12, 2015

@sebmarkbage

My HOC is a class, the problem is when the consumer wraps a stateless component.

Pseudocode:

// my library
function connect(WrappedComponent) {
  return class Connected {
    getWrappedInstance() { // people really asked for this method
     return this.refs.instance;
    }

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

// user code
connect(class Stuff { }) // good
connect(function StatelessStuff() { }) // emits warning although technically ref isn't even used
@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Oct 12, 2015

Ah, yes. That's the same problem that @yungsters has.

What methods are you expecting to forward on that ref? If you use a heuristic such as scanning the prototype, then if there is nothing on the prototype then you don't need to attach a ref because you don't have any methods to wrap.

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Oct 12, 2015

Oh I see, I didn't read properly. You use getWrappedInstance() instead of seamlessly exposing the methods like Relay does. In that case, it's not a seamless HoC anyway.

@gaearon
Copy link
Member

@gaearon gaearon commented Oct 12, 2015

I didn't know Relay forwards instance methods. I'll take a look, thanks!

@cpojer
Copy link
Member Author

@cpojer cpojer commented Oct 13, 2015

Relay doesn't forward instance methods in open source. We did this internally so the mixin -> container codemod would be safe. We didn't want to support this pattern externally however and we are hoping for a resolution to this issue inside of React.

@gaearon
Copy link
Member

@gaearon gaearon commented Oct 15, 2015

I solved my problem by making the ref opt-in on the HOC.
reduxjs/react-redux@2d3d0be

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 6, 2016

Posted some thoughts on this in #6974.

@slorber
Copy link
Contributor

@slorber slorber commented Jul 16, 2016

Hey,

Just want to say that also got this problem when using ReactIntl. Like Redux connect() it did provide a {withRef: true} option to pass to HOC: https://github.com/yahoo/react-intl/wiki/API#injectintl

Sometimes we have to espace from declarative programming for some reasons and provide component imperative API (in my case an image cropper component). It's quite annoying when someone just add some component i18n that it actually leads to a serious bug :)

ghengeveld added a commit to ghengeveld/react-isomorphic-form that referenced this issue Aug 31, 2016
CMTegner added a commit to reactjs/react-autocomplete that referenced this issue Sep 3, 2016
It's either this or `props.inputRef`, unless we want to wait for
facebook/react#4213
@the-spyke
Copy link

@the-spyke the-spyke commented Feb 20, 2017

I'm not sure that forwarding a ref is a comprehensive solution. For relatively simple HOCs it makes sense. But what if a HOC wants to add another instance method or maybe serve as an adapter/decorator? I think I need to:

  1. Choose some ref to get instance methods from
  2. Have an ability to filter out some of those methods
  3. Add my own or override some instance methods

Actually, there're lots of cases, when HOC should be indistinguishable from it's wrapped component for users. This reminds me inheritance in OOP, but for HOCs (what is a little bit dull knowing about OOP unpopularity these days).

@bradennapier
Copy link

@bradennapier bradennapier commented Oct 1, 2017

Hmm - not sure if this is possible with javascript directly -- but is it possible to have something somewhat similar in kin to componentDidCatch which would allow a HOC to "catch" accesses on undefined properties and choose how it wants to handle such a case?

I may be breaking a thousand javascript laws with this concept but it seems to help with these types of situations with other languages.

Perhaps something as simple as expecting a returned prop to either call or provider to the caller -- or expecting the method calls the function itself and returns the value, otherwise throwing the standard error(s)?

class ForwardUnknown extends Component {

  unknownPropAccessed(prop) {
    if (this._wrapped  && this._wrapped[prop]) {
      return this._wrapped[prop]
    }
  }

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

Something along these lines, I would think, provide the necessary options to allow a blind pass-through but also have the HOC intelligently handle the situation when said HOC is significantly more complex.

looks like http://2ality.com/2014/12/es6-proxies.html proxies could be used to provide this kind of functionality... albeit the performance hit is likely the problem

@ThomasRooney
Copy link

@ThomasRooney ThomasRooney commented Jan 31, 2018

I ended up writing the below to solve my problem. This will arbitrarily proxy methods not defined on this class to the wrapped element. I feel dirty but it works. This is inside a HOC that wraps a native element like 'input'. Definitely not fully precise, but good enough for most usecases (I use it with a custom transpiler that transparently does capture/replay testing across a react app).

    elementRef = (node) => {
      this.childNode = node;
      const oldProto = this.__proto__;
      this.__proto__ = new Proxy({}, {
        get(object, key) {
          if (oldProto[key]) {
            return oldProto[key];
          }
          if (node && node[key]) {
            if (typeof node[key] === 'function') {
              return node[key].bind(node);
            }
            return node[key];
          }
          return undefined;
        }
      });
    }

    render() {
      return React.createElement(nativeElement, {...this.props, ref: this.elementRef});
    }
@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 7, 2018

Still a work-in-progress, but I'm drafting an RFC for this here reactjs/rfcs#30

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Mar 30, 2018

An API for this has been added in React 16.3 released today: https://reactjs.org/blog/2018/03/29/react-v-16-3.html#forwardref-api.

@sophiebits sophiebits closed this Mar 30, 2018
Matchdev120 added a commit to Matchdev120/React-autoComplete that referenced this issue Apr 30, 2019
It's either this or `props.inputRef`, unless we want to wait for
facebook/react#4213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants