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

WIP RFC: Improvements to the "ref" system #11401

Closed
trueadm opened this Issue Oct 30, 2017 · 31 comments

Comments

Projects
None yet
7 participants
@trueadm
Copy link
Contributor

trueadm commented Oct 30, 2017

This is a formal discussion to talk about the future of refs within React and how we can improve upon them.

Current Behavior

Currently, there are two ways of doing refs in React, string refs and callback refs.

String refs

String refs can be applied to "composite" components that are class components (i.e. <MyComponent />) and "host" components (i.e. <span />).

An example of how this might look like for both types:

// host components
class MyComponent extends React.Component {
  componentDidMount() {
    this.refs.input.focus();
  }
  render() {
    return <div><input ref="input" type="text" /></div>
  }
}

// composite components
class InputWrapper extends React.Component {
  focus() {
    this.refs.input.focus();
  }
  render() {
    return <input ref="input" type="text" />
  }
}
class FormComponent extends React.Component {
  componentDidMount() {
    this.refs.inputWrapper.focus()
  }
  render() {
    return <InputWrapper ref="inputWrapper" />
  }
}

Callback refs

Callback refs can also be applied to "composite" components that are class components (i.e. <MyComponent />) and "host" components (i.e. <span />).

An example of how this might look like for both types:

// host components
class MyComponent extends React.Component {
  componentDidMount() {
    if (this._inputNode) {
      this._inputNode.focus();
    }
  }
  render() {
    return (
      <div>
        <input ref={domNode => this._inputNode = domNode} type="text" />
      </div>
    );
  }
}

// composite components
class InputWrapper extends React.Component {
  focus() {
    this._input.focus();
  }
  render() {
    return <input ref={domNode => this._input = domNode} type="text" />
  }
}
class FormComponent extends React.Component {
  componentDidMount() {
    this._inputWrapper.focus()
  }
  render() {
    return <InputWrapper ref={instance => this._inputWrapper = instance} />
  }
}

Proposed Behavior

I propose three major changes to how the current ref system works:

Deprecate string refs for removal in React 17

The ref API is broken is several aspects (taken from #1373).

  • 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 Flow or 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.
  • They require access to the React runtime to find the current owner during the creation of a ReactElement, making ahead-of-time optimizations hard to deal with.

Callback refs do not have the above issues and have been the recommended choice by the React team for some time. You can already do everything and more with callback refs, so I personally feel there's no need to keep the string ref system around.

Other libraries, such as Inferno and Preact have already removed string refs and have reported performance optimization from doing so.

Deprecate the "ref" prop entirely

I feel refs on components lead to problematic patterns that make apps much harder to scale because it can easily break the uni-direction flow of a component tree. In my opinion, class components shouldn't be able to access the instances of other components for communication – they should use props instead. Alternatively, in cases where access of a root DOM node is needed but unavailable, a wrapper component (#11401 (comment)) could be used as an escape hatch.

The below example is something that I personally feel is a problematic pattern and one that I've seen bite teams in the past:

class ItemContainer extends React.Component {
  render() {
    let { subscribe, unsubscribe } = props.SubscriptionHandler;

    return (
      <ul>
        { this.props.items.map( item => (
           <ListItem 
              key={item.uid}
              data={item.data}
              ref={
                _ref => _ref ? subscribe(item.uid, _ref) : unsubscribe(item.uid, _ref)
              }
           />
         ) }
      </ul>
    );
  }
}

The above example couples all the handling of the items in the item container, breaking the control flow. Ideally, the SubscriptionHandler should be passed to the child as a prop, then the child can control its own flow.

Another usage of refs on composite components is related to ReactDOM.findDOMNode(...) usage. By passing findDOMNode the component instance from the ref, you can get back the root DOM node. An example of this follows:

class DOMContainer extends React.Component {
  render() {
    if (this.props.type === "inline") {
      return <span />;
    } else {
      return <div />;
    }
  }
}
class Appender extends React.Component {
  componentDidMount() {
    ReactDOM.findDOMNode(this._domContainer).appendChild(this.props.node);
  }
  render() {
   return <DOMContainer ref={_ref => this._domContainer = _ref} type="inline" />
  }
}

This approach can be avoided in this instance by passing refs via props:

function DOMContainer(props) {
  if (props.type === "inline") {
    return <span ref={props.rootRef} />;
  } else {
    return <div ref={props.rootRef} />;
  }
}
class Appender extends React.Component {
  componentDidMount() {
     this._rootRef.appendChild(this.props.node);
  }
  render() {
   return <DOMContainer rootRef={_ref => this._rootRef = _ref} type="inline" />
  }
}

Add a special "hostRef" prop that only works on host components

This is to reduce confusion, as hostRef would be a normal prop on composite components. Keeping the current "ref" naming might cause unintended problems. This would also allow apps to move over to the new system incrementally. Furthermore, hostRef should only accept callback refs, not string refs. An example of this:

function Button({ className, ...props }) {
  return (
    <button
      {...props}
      className={classNames(className, 'my-btn')}
    />
  );
}

// "hostRef" is a simple prop here, and gets passed through to the <button> child via JSX spread
<Button hostRef={ _ref => console.log(_ref) } className="headerBtn" />

Downsides

Migration Cost

Both changes in this proposal have a cost for migration.

  • String refs are still widely used in third-party components but are likely to be trackable and upgraded via codemodding.
  • Refs on composite components are far more widely used than string refs, so it may not make sense to make those changes vs the cost it will have on the React ecosystem. It's unlikely that they can be upgraded via a codemod.

Codemodding

It may be possible to automate the vast majority of string refs to callback refs via a codemod. There will need to be some form of checking for where the owner of a ref differs in cases of string refs vs callback refs. [This point needs to be broken apart and discussed more]

It might not be possible to automate a codemod for refs on composite components as it would require a change in how the structure of the components in an app work. [This point needs to be broken apart and discussed more]

Other Considerations?

React Native currently doesn't have host components, only composite components. So refs on core components such as <View /> will need special consideration for how they may function as they do now. Maybe they could function by a prop called viewRef or something similar, which would work like refs currently do.

@trueadm trueadm changed the title RFC: Improvements to the "ref" system WIP RFC: Improvements to the "ref" system Oct 30, 2017

@kib357

This comment has been minimized.

Copy link

kib357 commented Oct 30, 2017

Deprecate refs on composite components
This approach can be avoided in this instance by passing refs via props:

Unfortunately, your solution isn't resolving problem of receiving component instance in parent component. For example, you have a component from other library/developer, and it hasn't any props for refs. You will need to do a PR, and it will be merged in a week, month, or never. So, it will break many of existing apps using refs without any other way to achieve this functionality.

@jquense

This comment has been minimized.

Copy link
Collaborator

jquense commented Oct 30, 2017

This approach can be avoided in this instance by passing refs via props:

I mentioned this a bunch on the eslint thread about deprecating findDOMNode with @taion but using props to pass refs for DOM node access is untenable for libraries that may not own either their parents or children. This approach only works if everyone agrees on the same prop names. Imagine if every library specified their own name for css classes, nothing would interop well because you'd need to handle class, className, classNames, cssClass etc. It's a bit less of a concern here, but it's still not great.

The root problem with composite refs seems to be it encourages using the instance imperatively, we could address that by having the same behavior as host refs, and always return the underlying host node from the ref, which is probably the 90% of the use case for refs anyway

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

There are alternative approaches to getting the root DOM node of a class component (for where you need it in special cases) via a wrapper component:

class RootNodeWrapper extends React.Component {
  componentDidMount() {
    this.props.rootRef(ReactDOM.findDOMNode(this))
  }
  componentWillUnmount() {
    this.props.rootRef(null)
  }
  render() {
    return <this.props.children />
  }
}

// usage
<RootNodeWrapper rootRef={...}><SomeChildComponent /></RootNodeWrapper>

There are other ways to do this without using findDOMNode too.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

Strongly agreed with #11401 (comment). The CSS class name example is on point.

These alternatives only work if there's some promulgated canonical name, like nodeRef or domRef or elementRef or something. Otherwise interop just becomes miserable and every library ends up inventing its own convention.

I do agree that always returning the host ref would be clever and neat, though. Still, the backdoor into getting the instance might be nice in a few cases. Not sure if cutting that off entirely is really best.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

I guess the ref system doesn't need to support that out-of-the-box. A parent could always pass down a callback that the child calls with this at render time.

So it'd be neat on web for ref to only pass up the host node.

Is that useful/meaningful in e.g. a React Native context though?

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

@taion Thinking about this, I'm not sure the naming is as big a problem. Looking at different codebases, the contract for passing though ref via explicit props seems more declarative in my opinion. Rather than a generic nodeRef or domRef, by specifying what type of node you want the ref for, say headerRef or buttonRef, it actually lends to being more descriptive. This is even more prevalent when a component render returns a fragment – in which case, there is no generic root DOM node to return (via findDOMNode).

As it's a composite component, you are free to name the prop as you want though, so you could use a generic name, even ref as that would no longer be a special case on composite components so legacy components should still work with this change as long as they pass down props.

@jquense I feel that having that logic for composite components will become even more confusing when developing in React Native. The API for refs should remain consistent across all renderers in my opinion.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

@trueadm That works if you have a single level of nesting. If you have multiple levels of nesting, it sort of breaks down. If I want my <Button> to work like a <button> or <a> for purposes of being able to receive a nodeRef or something such that I can generically attach a popover or tooltip to it, then a single nodeRef prop would be best – especially if also works for DOM components.

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

@taion By using a generic name like nodeRef you'd expect the callback to only return a single ref. This will likely become confusing because we've added fragments to React 16, so in theory there could be many refs. This might be something that react-call-return could better tackle in the future, but for now I still feel it's better to be explicit, or you can use ref and pass it through via a prop (but that would conflict).

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

I think in that case, you would just throw when attempting to attach nodeRef to something with fragments. It's not entirely ideal. Do you have a better suggestion for how to deal with this case? An example of where this comes up is something like:

<TooltipTrigger tooltip="do a foo">
  <Button>Foo</Button>
</TooltipTrigger>
<TooltipTrigger tooltip="do a foo">
  <a>Foo</a>
</TooltipTrigger>

Suppose <TooltipTrigger> needs to find the DOM node of the thing it's wrapping to do the position calculations for figuring out where to render the tooltip.

I'd like for there to exist a convention where this "just works", where someone implementing <Button> has a standard convention to follow.

Otherwise interop across component libraries becomes a hairy mess.

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

@taion In the case you have above, I'd wrap the child component in a <div> and use the calculations based off that. If that's not possible (you can't put an additional <div> in) then something like react-call-return is probably the better route or you could adapt my alternative wrapper component above using ReactDOM.findDOMNode.

I really expect fragments to become the most common return type for component renders in the future and throwing an error on them isn't ideal at all for a core API.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

It's not generally acceptable to add extra wrapping markup for this sort of utility component – and these components really do come up quite a lot. As a generic utility component, we can't e.g. assume that the user's component can just be wrapped with something that's display: block, without breaking their markup.

And while it may indeed be the case that most components will return fragments, it's also the case that most components don't need parents to inject e.g. event handlers into them to add e.g. "tooltip on hover" behavior. But components that care about this aren't going to return fragments anyway. That <Button> above will eventually render an <a> or a <button> or something, but that's not relevant to <TooltipTrigger> – all <TooltipTrigger> cares about is that <Button> does expose some API that lets it get a handle to a DOM element that it can use for position calculations. And we'd strongly rather this be something that takes a consistent convention, that makes it possible for <TooltipTrigger> to support both <Button> and <button>, without adding an extra <div> that the user never asked for.

While react-call-return could be interesting as a longer-term solution, its semantics are quite different, and it can't e.g. be shimmed. From the perspective of something like that <TooltipTrigger> above, assuming React 17 ships a fully-baked react-call-return, we don't really have great options for maintaining forward/backward compatibility. This has already bitten us to an extent with portals, but the old API at least was explicitly unstable. The ref API is not.

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

@taion In the case you expect a single component, then there's nothing stopping <TooltipTrigger> from using findDOMNode to get the DOM node. My proposal isn't suggesting we deprecate that API, not until we have a suitable replacement for escape hatches.

If you want to avoid findDOMNode and have something that can work with fragments, you could use a marker DOM node with a fragment. If you wanted to support a range of children, you could have a placeholder at the end too and then you could support handling many children refs.

class TooltipTrigger extends React.Component {
	componentDidMount() {
		this.props.ref && this.props.ref(this._markerNode.nextSibling);
	}
	componentDidUpdate() {
		this.props.ref && this.props.ref(this._markerNode.nextSibling);
	}
	render() {
		return [
			<span ref={_ref => this._markerNode = _ref} stlye={{display: "none"}} />,
			this.props.children,
		];
	}
}
@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

Your example per #11401 (comment) breaks down conceptually in the presence of fragment returns as well. And, I mean, that's fine – for my purposes here, I don't need a generic way to get a handle to or a DOM node for an arbitrary React element that may well be a fragment.

I see this pattern on the Facebook page, I think. It looks a little bit like:

<Tooltip tooltip="This is a tooltip">
  <Something />
</Tooltip>

It renders a tooltip in a portal (maybe not a React portal?) and positions it so that the tooltip shows up next to the <Something>.

This is a really common pattern generically. Almost every UI library exposes something like this, and often they don't make any assumptions beyond "the tooltip shows up on some element that can take mouse events" – certainly no mandatory extra markup.

So in this case what I do want is something that implicitly asserts that <Something> renders a single DOM element, and it should work if Something is literally just 'button'.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

Actually, is ref just going to become a normal prop on composite elements? In which case it'd serve the same role as "rootRef" in my hypothetical above – as in a composite component can choose to just forward it along if it wishes to expose a root node that way.

In which case, great, this is my preferred solution. Though I'm not sure how you'd handle that deprecation in such a way that doesn't warn for "legitimate" uses along those lines.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

As a motivating example, consider a component like:

function Button({ className, ...props }) {
  return (
    <button
      {...props}
      className={classNames(className, 'my-btn')}
    />
  );
}

This is a pretty natural way to write a component. If ref becomes just another prop in this case, then a parent doing <Button ref={(c) => { this.button = c; }}> gets exactly what it wants!

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

@taion Maybe we should deprecate ref entirely and use an alternative API to prevent confusion in that case – as ideally, in React 17, the equivalent of ref would just be a normal prop, but what I've trying to explain this whole time was that was I'm proposing actually fits better (like you showed above) than what we have now.

As this is still a WIP RFC, I'll update it tomorrow to reflect the name change. Personally, I feel it might be best to make ref go away in the cases above and instead have a ReactDOM specific escape-hatch on host nodes, maybe hostRef or domRef? hostRef would be a normal prop on composite components, meaning they can be passed through freely if needed. Thoughts?

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

Sure – so rename it to something like hostRef? Then on e.g. <span>, React would invoke the callback with the host node, while on everything else, it'd be just another prop?

I think that's something like what I was getting at earlier. I think it would be great – it ends up at what we were asking for in the ESLint discussion on findDOMNode, and for our purposes would replace findDOMNode.

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

^ that would be super sweet BTW. It'd be exactly what we wanted.

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Oct 30, 2017

I've updated the original post slightly but I'm a bit tired, so let me know if there's anything I've missed. I used your example too :)

@taion

This comment has been minimized.

Copy link

taion commented Oct 30, 2017

Looks great! Two small notes:

  • For "Other Considerations?", I think this handles the proposed viewRef case as well – RN composite components could just implement their own semantics for hostRef
  • I'd note that hostRef replaces findDOMNode for most (all? at least "all of our") remaining use cases for findDOMNode
@taion

This comment has been minimized.

Copy link

taion commented Jan 7, 2018

What's the current thinking here?

Would it make sense to make this an RFC?

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Jan 7, 2018

It would, maybe merge with the createRef RFC maybe. I’m not sure if I’ll get the time to start another RFC to do such things right now, I’m pretty busy working on the compilation work. Maybe someone else would like to push this through.

@taion

This comment has been minimized.

Copy link

taion commented Jan 7, 2018

I can write this up as an RFC. I'm going to follow along on #11973 for a bit first, though.

Does the OP here still reflect your thinking on this matter?

@trueadm

This comment has been minimized.

Copy link
Contributor

trueadm commented Jan 7, 2018

It does, although we need to incrementally move to a better ref system rather do we’re not surprising users. I believe adding createRef() and deprecated string refs is a good first step towards getting to that point.

@TrySound

This comment has been minimized.

Copy link
Contributor

TrySound commented Jan 7, 2018

@trueadm I believe just copying PR can be a good RFC. I got it purpose.

@sag1v

This comment has been minimized.

Copy link

sag1v commented May 7, 2018

@trueadm regarding your comment example, is there any other way to achieve this without findDOMNode?
Lets say you have a component that doesn't "know" what type of children it will receive and you want to attach or wrap it with a ref so you can capture clicks related to the children.
Code example:

class ExternalClick extends React.Component {
    state = {
        clickedOutside: false
    }

    componentDidMount() {
        this.ref = findDOMNode(this);
        document.addEventListener('mousedown', this.handleClickOutside);
    }

    componentWillUnmount() {
        document.removeEventListener('mousedown', this.handleClickOutside);
    }

    handleClickOutside = e => {
        if (this.ref && this.ref.contains) {
            const clickedOutside = !this.ref.contains(e.target);
            this.setState({ clickedOutside });
        }

    }

    render() {
        const { children } = this.props;
        const { clickedOutside } = this.state;
        
        if (typeof children === 'function') {
            return children(clickedOutside);
        } else {
            return null
        }
    }
}
@TrySound

This comment has been minimized.

Copy link
Contributor

TrySound commented May 7, 2018

@sag1v For this kind of api you should provide callback ref in children arguments.

class ExternalClick extends React.Component {
    state = {
        clickedOutside: false
    }

    registerChild = element => {
      this.ref = element;
      if (element) {
        document.addEventListener('mousedown', this.handleClickOutside);
      } else {
        document.removeEventListener('mousedown', this.handleClickOutside);
      }
    }

    handleClickOutside = e => {
        if (this.ref && this.ref.contains) {
            const clickedOutside = !this.ref.contains(e.target);
            this.setState({ clickedOutside });
        }
    }

    render() {
        const { children } = this.props;
        const { clickedOutside } = this.state;
        const { registerChild } = this;
        
        if (typeof children === 'function') {
            return children({ registerChild, clickedOutside });
        } else {
            return null
        }
    }
}
@sag1v

This comment has been minimized.

Copy link

sag1v commented May 7, 2018

@TrySound Thanks!
Yeah i thought about this option, and i think it's not ideal from the user's perspective.
I wouldn't want to force the user to use the ref API.
Though there is a benefit for this approach, the user can decide exactly what DOM node is targeted within the child component, but i think it's a nice to have as an extra feature / option and not as the core implementation.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Aug 15, 2018

Seems like forwardRef solved most concerns here so closing. If I'm wrong please open a more focused RFC for hostRef? Thanks!

@gaearon gaearon closed this Aug 15, 2018

@taion

This comment has been minimized.

Copy link

taion commented Aug 15, 2018

@gaearon The remaining question is the more ambitious one of "Deprecate the "ref" prop entirely" – are we convinced now that this is unnecessary?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Aug 15, 2018

I don’t think we really considered doing this. It’s important to have an imperative escape hatch, I think, and it doesn’t necessarily have to be tied to host instances. Any proposal for completely removing refs for custom components would need to consider a broad set of use cases, and provide alternatives for each of them. I don’t think this proposal has done it.

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