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: New createRef() API for ref objects #17

Merged
merged 11 commits into from
Feb 11, 2018
Merged

RFC: New createRef() API for ref objects #17

merged 11 commits into from
Feb 11, 2018

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Jan 25, 2018

A proposal for a new createRef api as a replacement for existing string refs.

Collected messages from this refs:

facebook/react#11555
facebook/react#11973
facebook/react#10581

Rendered

TODO

  • Alternatives
  • Drawbacks
  • Adoption strategy
  • Better "How we teach this"


# Basic example

The React.createRef() API will create an immutable object ref (where it's value is a mutable object referencing the actual ref). Accessing the ref value can be done via ref.value. An example of how this works is below:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change it's to its

@aulisius
Copy link

Hi,

How would this tie in with this.refs which is an iterable object? Would both ref APIs be supported? From what I gathered, the idea is to gradually replace string refs. Then, will this API provide such a method to implement the same?

Faizaan.

@j-f1
Copy link

j-f1 commented Jan 25, 2018

Alternate proposal:

class Component extends React.Component {
  onClear() {
    this.refs.input.value = ''
  }
  render() {
    return <div>
      <input ref={this.ref('input')}/>
      <a onClick={this.onClear}>Clear</a>
    </div>
  }
}

this.ref has the same behavior as a string ref, but without many of the caveats:

class Component {
  ref(name) {
    if (!this.ref._cache) {
      this.ref._cache = new WeakMap()
    }

    const { _cache: cache } = this.ref
    if (!cache.has(name)) {
      cache.set(name, ref => {
        this.refs[name] = ref
      })
    }

    return cache.get(name)
  }
}

@trueadm
Copy link
Member

trueadm commented Jan 25, 2018

@j-f1 I'd prefer if createRef() was not coupled to class components, plus your example doesn't look type safe as you're having to pass in a string to a function rather than access a property reference (which has the same problems as string refs).

Furthermore, I see a valid use case of createRef() to store refs on the state object for places where you might currently use instance variables.

@aulisius Why are you iterating on the refs object? Can you give me an example why you need to do this as it seems somewhat error prone.

@TrySound
Copy link
Contributor Author

@aulisius createRef produces a single value which will be mutated with single reference. So iteration is not the case anymore.

@j-f1
Copy link

j-f1 commented Jan 25, 2018

I'd prefer if createRef() was not coupled to class components

That’s a good idea, but this could be syntactic sugar only available to classes.

Also, how would a functional component use refs?

plus your example doesn't look type safe as you're having to pass in a string to a function rather than access a property reference

TypeScript:

// P = props
// S = state
// R = refs
class Component<P, S, R = {}> {
  // ...
  protected ref<K extends keyof R>(name: K): (ref: R[K]) => void
  protected refs: R
}

// Usage:

interface FooRefs {
  input: HTMLInputElement
  dialog: Dialog
}

class Foo extends React.Component<{}, {}, FooRefs> {
  renderInput() {
    return <input ref={this.ref('input')} />
  }
  renderDialog() {
    return <Dialog ref={this.ref('dialog')} />
  }
}

(of course, this needs some more typings at the JSX level)

@TrySound
Copy link
Contributor Author

TrySound commented Jan 25, 2018

@j-f1

That’s a good idea, but this could be syntactic sugar only available to classes.

I wouldn't add feature to remove it later just because it nice. Also react API always was small and simple. Feature duplicating is not good practice.

Also, how would a functional component use refs?

Nothing magical. Just pass ref as any prop and child will patch it. In componentDidMount you will always have filled reference.

@aulisius
Copy link

@j-f1 refs are not supported on SFC even now. Any reason that should change?

@TrySound I understood that. My question is, will there be an alternative to still be using the same pattern. As in,

class ComponentUsingRefs extends React.Component {
  componentWillReceiveProps(nextProps) {
    if (nextProps.isDataLoaded) {
      for (let refKey in this.refs) {
        const ref = this.refs[refKey];
        // Do stuff with it
      }
    }
  }

  render() {
    return this.props.list.map(values => (
      <ListItem key={values.id} ref={`${values.id}`} {...values} />
    ));
  }
}

How would do this code be written with the createRef API?

Faizaan.

@j-f1
Copy link

j-f1 commented Jan 25, 2018

Nothing magical. Just pass ref as any prop and child will patch it. In componentDidMount you will always have filled reference.

My proposal would work with a functional component, too:

function FunctionalComponent({ myRef }) {
  return <div ref={myRef} />
}

class ClassComponent extends React.Component {
  render() {
    return <FunctionalComponent myRef={this.ref('child')} />
  }
}

If this isn’t what you meant, can you give an example using the API in this RFC?

@streamich
Copy link

I kinda of like it, because it is kinda nice and convenient.

And I kinda dislike it, because it creates a 3rd interface for the thing we can already do in 2 ways.

@TrySound
Copy link
Contributor Author

@streamich It's a replacement for one those things. Also it's possible this feature can eliminate callback refs in the future.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Feb 3, 2018

It doesn't fully replace callback refs since these refs don't have a way to respond to a child being added and removed by a container.

They're only useful if you don't need to perform any effect and can lazily handle the case when a ref has been unmounted and remounted. That's most cases, but not all.

There are also cases where you can safely assume that a child shares the same life-time as the parent but sometimes that isn't fully safe. I suspect that with new types of containers like "expiration boundaries" this type of thing will happen more often than it does today.

So I don't see this replacing callback refs completely.

@gaearon
Copy link
Member

gaearon commented Feb 3, 2018

In other words the idea is to relegate callback refs to be a "power user" pattern for less common cases. This API would be a bit less powerful than callback refs but less cumbersome to use for the common case. It would replace string refs but stay alongside callback refs.

@TryingToImprove
Copy link

Would it make sense to also add a prop-type?

- PropTypes.Ref
- PropTypes.Ref.isRequired

@TrySound
Copy link
Contributor Author

TrySound commented Feb 6, 2018

@TryingToImprove Quite react specific. One of the case of splitting prop-types is reusing it with other libs. Refs can be achieved with simple PropTypes.object.isRequired. And you can always write custom prop type.

@TryingToImprove
Copy link

@TrySound There are already PropTypes.element. If the ref, is becoming a DOM element could also be PropTypes.DOMElement

@TrySound
Copy link
Contributor Author

TrySound commented Feb 6, 2018

@TryingToImprove Legacy)

@j-f1
Copy link

j-f1 commented Feb 6, 2018

There’s also PropTypes.node.

@j-f1
Copy link

j-f1 commented Feb 6, 2018

How about React.refPropType?

@TrySound
Copy link
Contributor Author

TrySound commented Feb 6, 2018

@j-f1 Of course not. prop-types is "external" feature. A lot of users don't need this api since they are use static typing.

@streamich
Copy link

Don't you need to use prop-types when using current context API.

@TrySound
Copy link
Contributor Author

TrySound commented Feb 6, 2018

@streamich I won't need it in the next week.

@j-f1
Copy link

j-f1 commented Feb 6, 2018

prop-types is "external" feature

React still checks propTypes when you provide them.

@gaearon
Copy link
Member

gaearon commented Feb 6, 2018

PropTypes is an external project to React now. This RFC doesn't need to concern itself with PropTypes. We can discuss it separately in PropTypes repo.

Why should we *not* do this? Please consider:

- implementation cost, both in term of code size and complexity
- whether the proposed feature can be implemented in user space
Copy link

@streamich streamich Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a user space implementation

const createRef = () => {
  const ref = el => ref.value = el;
  return ref;
};

React.createRef = createRef;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true. But in practice this pattern hasn’t caught on, and people keep using string refs in many places because they perceive functional refs as inconvenient. By adding this into the core we encourage people to adopt this pattern more strongly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon Deprecation warning with link on documentation should have a similar effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we deprecate string refs before adding this helper people will get very annoyed. Because function refs are more cumbersome to deal with. Yes, we can suggest this helper. But at that point we’re suggesting everybody to copy the same thing into their projects. What’s the point? It might as well be provided by React then.

@streamich
Copy link

streamich commented Feb 7, 2018

@strayiker

inputRef = ref => { this.input = ref };
inputRef = React.createRef();

-10 characters on defining

inputRef = e => this.input = e;
inputRef = React.createRef();

-2 characters on defining

@TryingToImprove
Copy link

TryingToImprove commented Feb 7, 2018

Is this something that is going to be allowed?

class Parent extends Component {
    constructor() {
        this.childRef = React.createRef()
    }

    componentDidMount() {
        //////////---------------- NOTE: that the childRef come from another component
        /// this.childRef === document.getElementById('HELLO_WORLD') // true
    }

    render() {
        return <Child passedRef={this.childRef} />
    }
}

class Child extends Component {
    render() {
        return ( 
            <div>
                <span id="HELLO_WORLD" ref={this.props.passedRef}
            </div>
        )
    }
}

Seems like it is not possible with string-ref? Callback-ref seems to have the same "feature"

And if it is going to be allowed, how would it handle conditional rendering?

someCondition ? <ChildA passedRef={this.childRef} /> : <ChildB passedRef={this.childRef} />

Seems like it is going to create memory-leaks since the Parent will hold the reference to the DOM element when a Child get removed.

@TrySound
Copy link
Contributor Author

TrySound commented Feb 7, 2018

Nope, reference value will be set to null by that child

@gaearon
Copy link
Member

gaearon commented Feb 7, 2018

You can pass an object ref down the tree (just like a callback ref), but it doesn’t create memory leaks because React clears its value when the ref is detached.

}

componentDidMount() {
this.divRef.value.focus();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this.divRef() as a shortcut? People are going to be typing this a lot, and that extra .value is 4 characters longer than (). You could define React.createRef as something like this:

function createRef() {
  const ref = () => ref.value;
  ref.value = null;
  ref.__THIS_IS_A_REACT_REF_DO_NOT_TOUCH_THIS_OR_YOU_WILL_BE_FIRED__ = true;
  return ref;
}

Copy link
Member

@gaearon gaearon Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If object ref is also a function, there’s no easy way to distinguish which ref is which kind. Sure, React could use a field for this, but it is pretty confusing for third party libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also refs are an escape hatch. People shouldn’t be typing this so much.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for __THIS_IS_A_REACT_REF_DO_NOT_TOUCH_THIS_OR_YOU_WILL_BE_FIRED__ almost spilled my hot chocolate.

@trueadm
Copy link
Member

trueadm commented Feb 7, 2018

In regards to comparing the amount of characters to type – I think you're missing the point with this PR. createRef isn't meant to replace callback refs, if you like using callbacks refs, please continue to use them. The issue is that there are plenty of people out there who still use string refs because they dislike using callback refs and this RFC aims to address that target audience.

I've had the pleasure of attending React bootcamp classes and introduction sessions where newcomers to React start out. One of the common complaints is that callback refs are far too confusing when learning React compared to string refs.

When I explain to them they can do (divRef) => this._divRef = divRef or something similar, they go on to ask questions like:

  • "How do I add a debugger into the arrow function?"
  • "I thought creating closures in the render method was expensive?"
  • "Why is the ref called multiple times with divRef being different values?"

Obviously, you can tackle all those points and address them, but most people then just use string refs without the hassle.

@TrySound
Copy link
Contributor Author

TrySound commented Feb 7, 2018

@trueadm Are you ok to chat in twitter PM?

@@ -26,6 +26,25 @@ class MyComponent extends React.Component {

# Motivation

Strings refs bind to the React's component's `currentOwner` rather than the parent. That's something that isn't statical analysable and leads to most of bugs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most people don’t know what “current owner” is. Can you please explain the concept here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to describe it. Doesn't example below help?

}
}
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mention other problems with string refs? For example lack of composability, or that they break “multiple Reacts” case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "lack of composability" and how createRef fixes it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what is the symptom of breaking with multiple reacts. Is there an error or just refs are not filled?

}
```

This alternative API shouldn't provide any big real wins over callback refs - other than being a nice convenience feature. There might be some small wins in performance - as a common pattern is to assign a ref value in a closure created in the render phase of a component - this avoids that (even more so when a ref property is assigned to a non-existent component instance property).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add here that the primary motivation is to encourage people to migrate off string refs. Callback refs meet some resistance because they are a bit harder to understand. We want to introduce this API primarily for people who love string refs today.

@@ -49,6 +49,12 @@ class ComponentA {
}
```

It is not statical analysable and leads to most of bugs.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statically-analyzable

?


Initially, we will release the object refs alongside the existing string refs as a minor update. Also this update will include `StrictMode` which enables deprecation messages for its subtree, so people be able to catch using string refs in their components and replace with the new api incrementally.

String refs will be removed in upcoming major release, so there will be enought time for migration.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enought -> enough?

@x5engine
Copy link

x5engine commented Aug 29, 2018

Oh my God I wasted so much time because of this because it is returning null half of the time in componentDidMount and is accessible only after a few seconds not right away!

What can't we just use the normal document.getElementById ? it was first and simple to do!

This is returning null most of the time! (sometimes I this.myRef.current to be defined)

Please test before approving something!

THIS facebook/react#9328

@TrySound
Copy link
Contributor Author

@meteorplus Everything is tested and works perfectly. You probably missed the point that refs inside conditions are available only when conditions are thruthy. To cover all cases with this api you should use both componentDidMount and componentDidUpdate.

class A extends React.Component {
  div = React.createRef();
  state = {
    show: false
  }

  componentDidMount() {
    this.div.current // null because on component mount div is not rendered
  }

  componentDidUpdate() {
   // after click on button div element will be shown
    this.div.current // inside this hook it will be filled
  }

  render() {
    return (
      <>
        <button onClick={() => this.setState({ show: true })}>Click open</button>
        {this.state.show &&
          <div ref={this.div}></div>
        }
      </>
    )
  }
}

getElementById is a bad option because it requires to introduce unique identifiers which breaks component isolation.

@reactjs reactjs locked as off-topic and limited conversation to collaborators Aug 29, 2018
@gaearon
Copy link
Member

gaearon commented Aug 29, 2018

@meteorplus If you experience a bug in React please file a new issue with reproducing example. An RFC is not an appropriate place to express your frustration. Thanks!

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

Successfully merging this pull request may close these issues.