Skip to content

[Fabric] Don't pass instanceHandle to clones#13125

Merged
sebmarkbage merged 1 commit intofacebook:masterfrom
sebmarkbage:dontpassinstancehandle
Aug 16, 2018
Merged

[Fabric] Don't pass instanceHandle to clones#13125
sebmarkbage merged 1 commit intofacebook:masterfrom
sebmarkbage:dontpassinstancehandle

Conversation

@sebmarkbage
Copy link
Contributor

@sebmarkbage sebmarkbage commented Jun 29, 2018

I changed my mind from #12824. We will instead just reuse the first one.

It turns out that this is kind of costly to do cross-VMs. Additionally, we can't really safely use this mechanism to extract the props since we pool the Fibers (so it's not safe to read from an old Fiber's memoizedProps to extract event listeners). To make it safe we'd also have to pass props which are immutable but then that is even more costly.

Finally, in the cases where this matters - async dispatching of events - it doesn't really matter which one we use since the async dispatching nature means that they have to be resilient regardless.

NOTE: We can't actually land this until the native changes has fully propagated everywhere.

We will instead just reuse the first one.
@sebmarkbage sebmarkbage requested a review from gaearon June 29, 2018 04:03
@sebmarkbage sebmarkbage changed the title Don't pass instanceHandle to clones [Fabric] Don't pass instanceHandle to clones Jun 29, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jun 29, 2018

OK. Can you explain more about how you plan to "reuse the first one"?

@sebmarkbage
Copy link
Contributor Author

See #13024

We use the canonical object to read the current props.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@nhunzaker
Copy link
Contributor

nhunzaker commented Aug 3, 2018

@sebmarkbage anything preventing this from merging?

Sorry. I should read.

@nhunzaker
Copy link
Contributor

Well shoot, sorry for the noise. I missed the last line of your comment.

@facebook-github-bot
Copy link
Contributor

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

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2018

@facebook-github-bot chill please

@nhunzaker
Copy link
Contributor

I summoned the Kraken 😨

@facebook-github-bot
Copy link
Contributor

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

4 similar comments
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@sebmarkbage sebmarkbage merged commit 4fa20b5 into facebook:master Aug 16, 2018
@mrshelly

This comment has been minimized.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants