jquense / react-dom-lite

Tiny dom implementation using react-reconciler
246 stars 21 forks source link

flow fixes #9

Closed jquense closed 6 years ago

jquense commented 6 years ago

Still not correct, since the host config expects all things implemented. This is a great example of what I was talking about, instead of implementing controlled components i spent two hours configuring flow and ran out of time

ManasJayanth commented 6 years ago

What was the flow error? Is this branched off the flow branch? Last I checked there were no flow errors there.

jquense commented 6 years ago

Your PR didn't have a flow pragma on the reconciler types, so it wasn't being typed checked at all

ManasJayanth commented 6 years ago

@jquense So flow wasn't type checking HostConfig at? I skipped the pragma on purpose. That file is a stopgap fix. Those types are supposed to come from upstream. Flow checking that file would be like unit testing library code.

I did try passing wrong types to the HostConfig object. Flow did complain. Was it not complaining for you?

Edit: *come from react-reconciler package.

jquense commented 6 years ago

So if the file isn't checked it's the same as if the types weren't there. The HostConfig type was any, which defeats most of the purpose of having it, since weren't getting any type safety in index.js

Generally the stopgap solution for missing types in dependencies is a lib def file in flow-typed/ I started to put one together, but it's not complete

ManasJayanth commented 6 years ago

HostConfig was not annotated any

jquense commented 6 years ago

Sorry, I know that. But flow could not determine the type because the type file was broken, so regardless of what the annotation said. Flow reported the value as any. Do you have editor support for flow type? You usually can inspect the type inference flow or turn on flow coverage you'd see that the types were not doing what we'd want

jquense commented 6 years ago

Sorry if I'm not not doing a good job explaining the issue!

ManasJayanth commented 6 years ago

Hey, not sure if this helps. Nevertheless, I put together these types in gist You seem to have done more work though.

jquense commented 6 years ago

should be good to go now

jquense commented 6 years ago

this look good for now?