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

Update Flow and Fix Hydration Types #11493

Merged
merged 7 commits into from Nov 12, 2017

Conversation

Projects
None yet
3 participants
@sebmarkbage
Member

sebmarkbage commented Nov 8, 2017

This is an alternative to #11386.

@gaearon and @calebmer did some digging into why we were seeing inference errors and noticed that getNextHydratableSibling and getFirstHydratableChild returns a union that is ambiguous when inferred.

This highlighted that this API was already poorly defined. This fixes that.

First, we had no refinement on the hydratable child so we just assumed that if canHydrateInstance returns true then it is an instance suitable for that Fiber. That only worked because stateNode is any. This adds a specific refinement and the ability for that instance to be different than the first read. This could be important when the hydratable thing is part of a boxed variant or something.

Another thing that this highlighted is that there isn't really any difference between an Instance and a TextInstance. They could be the same type and probably often is in other models.

So this PR also introduces a Hydratable Instance type which is really a placeholder value for what we will later refine to an Instance or a TextInstance. This lets something that is both an Instance and a TextInstance be passed along here and only when we try to hydrate it do we define it as one or the other.

@sebmarkbage sebmarkbage requested a review from gaearon Nov 8, 2017

@gaearon

gaearon approved these changes Nov 8, 2017

Nice. I understand this fix.

gaearon and others added some commits Oct 27, 2017

Fix createElement() issue
The * type was too ambiguous. It's always a string so what's the point?

Suppression for missing Flow support for {is: ''} web component argument to createElement() didn't work for some reason.
I don't understand what the regex is testing for anyway (a task number?) so I just removed that, and suppression got fixed.
Expand the unsound isAsync check
Flow now errors earlier because it can't find .type on a portal.
Introduce "hydratable instance" type
The Flow error here highlighted a quirk in our typing of hydration.
React only really knows about a subset of all possible nodes that can
exist in a hydrated tree. Currently we assume that the host renderer
filters them out to be either Instance or TextInstance. We also assume
that those are different things which they might not be. E.g. it could
be fine for a renderer to render "text" as the same type as one of the
instances, with some default props.

We don't really know what it will be narrowed down to until we call
canHydrateInstance or canHydrateTextInstance. That's when the type is
truly refined.

So to solve this I use a different type for hydratable instance that is
used in that temporary stage between us reading it from the DOM and until
it gets refined by canHydrate(Text)Instance.
Have the renderer refine Hydratable Instance to Instance or Text Inst…
…ance

Currently we assume that if canHydrateInstance or canHydrateTextInstance
returns true, then the types also match up. But we don't tell that to Flow.

It just happens to work because `fiber.stateNode` is still `any`.

We could potentially use some kind of predicate typing but instead
of that I can just return null or instance from the "can" tests.

This ensures that the renderer has to do the refinement properly.

@sebmarkbage sebmarkbage merged commit acabf11 into facebook:master Nov 12, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon referenced this pull request Nov 13, 2017

Closed

Update Flow #11386

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Update Flow and Fix Hydration Types (facebook#11493)
* Update Flow

* Fix createElement() issue

The * type was too ambiguous. It's always a string so what's the point?

Suppression for missing Flow support for {is: ''} web component argument to createElement() didn't work for some reason.
I don't understand what the regex is testing for anyway (a task number?) so I just removed that, and suppression got fixed.

* Remove deleted $Abstract<> feature

* Expand the unsound isAsync check

Flow now errors earlier because it can't find .type on a portal.

* Add an unsafe cast for the null State in UpdateQueue

* Introduce "hydratable instance" type

The Flow error here highlighted a quirk in our typing of hydration.
React only really knows about a subset of all possible nodes that can
exist in a hydrated tree. Currently we assume that the host renderer
filters them out to be either Instance or TextInstance. We also assume
that those are different things which they might not be. E.g. it could
be fine for a renderer to render "text" as the same type as one of the
instances, with some default props.

We don't really know what it will be narrowed down to until we call
canHydrateInstance or canHydrateTextInstance. That's when the type is
truly refined.

So to solve this I use a different type for hydratable instance that is
used in that temporary stage between us reading it from the DOM and until
it gets refined by canHydrate(Text)Instance.

* Have the renderer refine Hydratable Instance to Instance or Text Instance

Currently we assume that if canHydrateInstance or canHydrateTextInstance
returns true, then the types also match up. But we don't tell that to Flow.

It just happens to work because `fiber.stateNode` is still `any`.

We could potentially use some kind of predicate typing but instead
of that I can just return null or instance from the "can" tests.

This ensures that the renderer has to do the refinement properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment