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

Initial implementation of new-style refs #1554

Merged
merged 1 commit into from
Nov 6, 2014

Conversation

sophiebits
Copy link
Collaborator

Depends on/includes #1758.

cf. #1373

This particular implementation doesn't attempt to do anything smart about async resolution -- .then always runs one of its arguments immediately. It can be used in any situation that refs can currently be used (and can also be used without an owner, which is a plus).

We can enhance this later to do something more intelligent when using refs during updates.

cc @jordwalke @sebmarkbage

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

* @param {function} success Callback in case of success
* @param {?function} failure Callback in case of failure
*/
then(success, failure) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One cool feature of the async refs, is that it makes it impossible (or at least difficult/fragile) for anyone to access a ref inside of a render method.

@syranide
Copy link
Contributor

@jordwalke
Copy link
Contributor

So it sounds like the only thing left to do on this one is to make the refs .then actually asynchronous (on a different event loop) so that program behavior is predictable. Was that @sebmarkbage's only concern?

@sophiebits
Copy link
Collaborator Author

I think so. You also wanted some test cases around cloneWithProps and transferPropsTo. Do you want to support having more than one ref? Right now cloneWithProps just warns you if you clone a component with a (string) ref.

@jordwalke
Copy link
Contributor

With this diff, cloning a component with (this type of) ref should be okay because refs won't be tied to any particular owner. So that means multiple components can retain a reference to the ref.
Yeah, some test cases. But also - it would be great to not put the refs callbacks on another event loop cycle - that can be pretty far in the future (many ms). So can we figure out a way to get them to work like setImmediate?

I'd say it's bad for any program to be effected by whether or not your refs are resolved in a blocking manner, (at the moment you invoke .then()). But it's probably not as bad for program behavior to be effected by whether or not your ref is resolved at a later time towards the end of the current event loop, or on a different event loop.

So that being said, @sebmarkbage: Can we just make sure the then() is resolved later, but no ton a different event loop? Then this should be fairly compatible with whatever promise library we pull in. If it doesn't have completely identical behavior, at least it will be very close.

@jordwalke
Copy link
Contributor

Under those terms, we can hide this feature behind React.__usePromiseRefs = true/false that I can flip to try it out.

@sophiebits
Copy link
Collaborator Author

This doesn't work on initial render because of how the batching currently works and also doesn't work if you access it outside of a lifecycle method, like in an event handler. Not sure how serious a problem that is for you @jordwalke.

zpao pushed a commit to zpao/react that referenced this pull request Jul 21, 2014
Callbacks passed to this setImmediate function are called at the end of the current update cycle, which is guaranteed to be asynchronous but in the same event loop (with the default batching strategy).

This is useful for new-style refs (facebook#1373, facebook#1554) and for fixing facebook#1698.

Test Plan: jest
sophiebits added a commit to sophiebits/react that referenced this pull request Jul 22, 2014
Callbacks passed to this setImmediate function are called at the end of the current update cycle, which is guaranteed to be asynchronous but in the same event loop (with the default batching strategy).

This is useful for new-style refs (facebook#1373, facebook#1554) and for fixing facebook#1698.

Test Plan: jest
@sebmarkbage
Copy link
Collaborator

Let's call setImmediate something else (such as asap). setImmediate doesn't do exactly what we want here and probably won't be standardized. Something else will be standardized with a different name, so the upgrade path to a real polyfill is confusing.

Also, I think that we should make _owner + string an API of createElement that creates a first-class ref behind the scene that updates this.refs. That way we can deprecate the old style in the core.

I still hate this API (even though I originally came up with it) but I don't have a better idea for now.

cf. facebook#1373

This implementation can be used in any situation that refs can currently be used (and can also be used without an owner, which is a plus).
@sophiebits sophiebits merged commit 9edc626 into facebook:master Nov 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants