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

Update Flow #11386

Closed
wants to merge 7 commits into from
Closed

Update Flow #11386

wants to merge 7 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 27, 2017

See individual commits.

I had to explicitly annotate ReactDOM host config because otherwise the inference wasn’t good enough.

I actually kind of like it this way because in the past it was often unclear where the inference failed.
Now it’s explicit.

I can update other renderers too if you want but this is enough to get it passing.

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.
Flow now errors earlier because it can't find .type on a portal.
This seems to help Flow inference.
@sebmarkbage
Copy link
Collaborator

Annotating the host config is a no-go imo. Updating renderers downstream that way is such a pain to maintain since the generic list is so big and difficult to update. Especially when you get it wrong

It's probably an indicator that something is not properly expressed in the host config itself. I'd like to look at that a bit further. I'd rather express that we'll in the host config types and solve it for all downstreams.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 27, 2017

I spent an hour and a half there and couldn't get it to work. If you can, please do 😛

The most I could narrow it down is that typing getNextHydratableSibling and getFirstHydratableChild as returning any cuts each ~8 issues (with both going down to 0). Something's up with those I | CI types and how they flow into everything else.

It could also be a bug in Flow.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 27, 2017

Annotating the host config is a no-go imo. Updating renderers downstream that way is such a pain to maintain since the generic list is so big and difficult to update. Especially when you get it wrong

I disagree. I found the opposite to be true: if there's a subtle error in some return type, the messages are so cryptic I need to go through the whole file and check every generic type by hand anyway. I prefer to see it explicitly and thus aid Flow in better messages. Since if I create a renderer I want to be aware of the exact contract I'm implementing, and my types as well.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 13, 2017

Rolled into #11493.

@gaearon gaearon closed this Nov 13, 2017
@gaearon gaearon deleted the flow branch November 13, 2017 15:45
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.

3 participants