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.

Sign up for GitHub

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

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2647d5d
RFC: New createRef() API for ref objects
TrySound Jan 25, 2018
0b0d5d9
Tweak typo
TrySound Jan 25, 2018
ea8070b
Improve motivation
TrySound Feb 9, 2018
c709f3e
Add drawback
TrySound Feb 9, 2018
7d0d3c4
Add alternatives and adoption strategy sections
TrySound Feb 9, 2018
e249d45
Improve motivation
TrySound Feb 9, 2018
9ae1b73
Add more string ref problems
TrySound Feb 9, 2018
3bfb605
Tweak word
TrySound Feb 9, 2018
81a7d52
Tweak word
TrySound Feb 9, 2018
f9c52ed
Reword the RFC
gaearon Feb 11, 2018
a8ccc5a
Bookkeeping
gaearon Feb 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion text/0000-new-create-ref.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ The primary motivation is to encourage people to migrate off string refs. Callba

There's a few problems with them.

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.
Strings refs bind to the React's component's `currentOwner` rather than the parent. This breaks "render prop" pattern.

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

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?

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

?


It requires that React keeps track of currently rendering component (since it can't guess this). This makes React a bit slower.

Another problem is breaking with two instances of react if for some reason react packages weren't deduped.

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.


One benefit would be more correct Flow types. People tend to type refs incorrectly because Flow doesn't enforce uninitialized class properties correctly:
Expand Down