-
Notifications
You must be signed in to change notification settings - Fork 560
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
Conversation
text/0000-new-create-ref.md
Outdated
|
||
# 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: |
There was a problem hiding this comment.
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
Hi, How would this tie in with Faizaan. |
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>
}
}
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)
}
} |
@j-f1 I'd prefer if Furthermore, I see a valid use case of @aulisius Why are you iterating on the |
@aulisius |
That’s a good idea, but this could be syntactic sugar only available to classes. Also, how would a functional component use refs?
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) |
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.
Nothing magical. Just pass ref as any prop and child will patch it. In |
@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 Faizaan. |
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? |
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. |
@streamich It's a replacement for one those things. Also it's possible this feature can eliminate callback refs in the future. |
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. |
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. |
Would it make sense to also add a prop-type?
|
@TryingToImprove Quite react specific. One of the case of splitting prop-types is reusing it with other libs. Refs can be achieved with simple |
@TrySound There are already |
@TryingToImprove Legacy) |
There’s also |
How about |
@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. |
Don't you need to use |
@streamich I won't need it in the next week. |
React still checks |
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. |
text/0000-new-create-ref.md
Outdated
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 |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
inputRef = e => this.input = e;
inputRef = React.createRef(); -2 characters on defining |
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 And if it is going to be allowed, how would it handle conditional rendering?
Seems like it is going to create memory-leaks since the |
Nope, reference value will be set to null by that child |
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. |
text/0000-new-create-ref.md
Outdated
} | ||
|
||
componentDidMount() { | ||
this.divRef.value.focus(); |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
In regards to comparing the amount of characters to type – I think you're missing the point with this PR. 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
Obviously, you can tackle all those points and address them, but most people then just use string refs without the hassle. |
@trueadm Are you ok to chat in twitter PM? |
text/0000-new-create-ref.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
text/0000-new-create-ref.md
Outdated
} | ||
} | ||
``` | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
text/0000-new-create-ref.md
Outdated
} | ||
``` | ||
|
||
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). |
There was a problem hiding this comment.
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.
text/0000-new-create-ref.md
Outdated
@@ -49,6 +49,12 @@ class ComponentA { | |||
} | |||
``` | |||
|
|||
It is not statical analysable and leads to most of bugs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statically-analyzable
?
text/0000-new-create-ref.md
Outdated
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enought -> enough?
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 |
@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 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>
}
</>
)
}
}
|
@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! |
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