make sure files use flow annotation

joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js

ISC License

3.27k stars 154 forks source link

make sure files use flow annotation #293

Closed mathieudutour closed 5 years ago

mathieudutour commented 5 years ago

Summary: Some files used flow types without the flow annotation so it wasn't actually checking anything. This PR fixes it and enable an eslint rule to make sure it doesn't happen again

[x] all files use flow
[x] add eslint rule to make sure of the above in the future
[x] remove last action creator using a thunk
[x] all components using a dispatched action creator now have types that reference the action creator directly
[x] all reducers have a proper Action type
[x] all sagas have a proper Action type
[x] update flow (and eslint to support the new flow version) so handle the new actions types

mathieudutour commented 5 years ago

I'd even advocate for enabling 'plugin:flowtype/recommended' but it shows errors when using any so there are tons of them. Probably another separate task

j-f1 commented 5 years ago

You could override the error so it’s only a warning.

mathieudutour commented 5 years ago

I could but it'd be better to fix them :)

joshwcomeau commented 5 years ago

Sorry for the delay on this, I haven't had much time recently. At a glance the changes look good, but I haven't gone through them thoroughly yet.

I'd push back a bit on plugin:flowtype/recommended - I feel like having 0 any is a bit like having 100% test coverage... it's nice to have, but at a certain point, the added friction is just not worth the marginal benefits. 80% seems fine to me. Although I suppose we could disable the errors in cases where we really didn't think it was worth it to define a valid type... I don't have super strong feelings on the matter.

mathieudutour commented 5 years ago

Indeed, I didn't add plugin:flowtype/recommended for that reason: I don't want to add friction. All the types I added here are really easy to add when adding a new feature because they are all inferred from the action creator.

mathieudutour commented 5 years ago

This really should have been merged...

I just rebased my fork with the latest master from guppy and pretty much all the new files don't have the flow annotation and have a bunch of flow errors when I add it