Fix a few Flow complaints we'd get with RN 0.63 and Flow 0.122.0. #4318
Conversation
|
Hmm, that CI failure might be transient: (lots of Unimodules output) |
There was a problem hiding this comment.
Thanks @chrisbobbe ! Small comments below; after taking care of them, please merge at will.
I agree, that error message looks like a transient issue -- either at dl.google.com, or in Travis CI, or in the network between them. (So probably Travis.) Tests all pass for me locally now.
| /* eslint-disable flowtype/generic-spacing */ | ||
| style?: SubsetProperties< | ||
| ViewStyle, | ||
| {| | ||
| marginTop?: mixed, | ||
| |}, | ||
| >, |
There was a problem hiding this comment.
Partly in the spirit of 8b30fa9d6, but also because this helps us
avoid a mystifying error reported by Flow when we take the upgrade
to Flow v0.122.0, with the React Native v0.63 upgrade (#4245).
I was seeing the error on the use of `ZulipButton` in `AuthScreen`;
it didn't like what I was passing for the `style` prop. When I
temporarily commented out the `style` prop passed to
`IosCompliantAppleAuthButton`, the error disappeared. It doesn't
make a lot of sense to me, but it sounds like
`IosCompliantAppleAuthButton`'s type for its `style` prop was
influencing what type `styles.halfMarginTop` was inferred to have,
when it was passed to `ZulipButton`.
Huh wacky. That seems like a Flow bug... quite likely reporting a real type mismatch, but a bug in at least where it places the error. I'd be curious to take a look at the error -- want to paste it somewhere, perhaps in chat?
In any case this seems like a fine tightening of the type. Perhaps add a comment pointing at ZulipButton's props, where a comment discusses this idea.
There was a problem hiding this comment.
Sure, I've just posted the error(s!) in chat, here.
| <Image | ||
| source={{ uri: sharedData.sharedImageUrl }} | ||
| width={200} | ||
| height={200} | ||
| style={styles.imagePreview} | ||
| /> |
There was a problem hiding this comment.
Worth a quick mention in the commit message that this isn't a change in Flow -- these props are there in the type in RN at our current version, and it's that type definition in RN that's getting stricter in the upgrade, to match the docs as you describe.
Here's the relevant bit of the type definition as of our current v0.62.x version:
/**
* See https://facebook.github.io/react-native/docs/image.html#style
*/
style?: ?ImageStyleProp,
// Can be set via props or style, for now
height?: ?DimensionValue,
width?: ?DimensionValue,| @@ -83,7 +83,6 @@ untyped-type-import=warn | |||
| nonstrict-import=warn | |||
| deprecated-type=warn | |||
| unsafe-getters-setters=warn | |||
| inexact-spread=warn | |||
There was a problem hiding this comment.
Here's the explanation in the linked RN commit facebook/react-native@87370c4c0 :
After changes to the semantics of spread, this lint is no longer relevant and will be removed in an upcoming version of Flow.
Based on that, I think ideally we'd remove this after the upgrade -- that way we have the lint as long as we're still on versions of Flow where it might be relevant.
Is it causing an error or warning at the upgrade? If so it'd be fine to remove it before, but probably I'd still want it as part of the same PR/branch as the upgrade.
... OK, I went and did a quick history search (git log --stat -p -S inexact-spread --pickaxe-all) in the Flow repo, and it looks like
- the lint was outright removed, so it probably becomes an error to have it in .flowconfig, in v0.117 at facebook/flow@759970c;
- the lint had already stopped doing anything in v0.111 at facebook/flow@0410e05 -- or at least, it had stopped ever firing in any situation exercised by Flow's tests. (The later commit that removed it didn't have to touch the tests, except to remove the lint from a flowconfig.)
We're on v0.113.0, in between those -- so in fact now is a perfect time to remove this lint from our flowconfig.
(Seems like an oversight that this isn't mentioned in the Flow changelog, at https://github.com/facebook/flow/blob/master/Changelog.md#01170 .)
Fixing these will end up saving us from quite a lot of Flow errors when it's time to upgrade Flow to 0.122.0, which we'll do with the React Native v0.63 upgrade (zulip#4245). Yay!
…le`. Partly in the spirit of 8b30fa9, but also because this helps us avoid lots of mystifying error reports from Flow when we take the upgrade to Flow v0.122.0, with the React Native v0.63 upgrade (zulip#4245). See discussion of those errors [1], and Greg's conclusion that Flow might have been a bit more helpful than it was here. Ah, well. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20errors.20in.20.23M4318/near/1075420
As of RN v0.62.2, which we're on currently, we've technically been allowed to choose between setting `width` and `height` as their own props, or putting them in the `style` prop. Here's the relevant bit of the type definition: ```js /** * See https://facebook.github.io/react-native/docs/image.html#style */ style?: ?ImageStyleProp, // Can be set via props or style, for now height?: ?DimensionValue, width?: ?DimensionValue, ``` But the `width` and `height` props aren't in the documentation [1]; it seems that the most proper place for them is in the `style` prop. And when we upgrade to RN v0.63, which we hope to do soon, we'll no longer be allowed to choose; using the `style` prop will be the only option. We've already been passing `width` and `height` in the `style` prop, so just fall back to doing that by itself. [1] https://reactnative.dev/docs/0.62/image
chrisbobbe
force-pushed
the
pr-flow-pre-rn-63
branch
from
dc73a2b
to
2ba4aaa
Compare
Part of the RN v0.62 -> v0.63 changes to the template app [1], corresponding to facebook/react-native@87370c4c0. This lint rule was removed in facebook/flow@759970c1b, which was released in Flow v0.117. So we'd get an error if we left this rule in when taking Flow v0.122.0 along with the RN upgrade. But as Greg points out [2]: """ the lint had already stopped doing anything in v0.111 at facebook/flow@0410e0502 -- or at least, it had stopped ever firing in any situation exercised by Flow's tests. (The later commit that removed it didn't have to touch the tests, except to remove the lint from a flowconfig.) """ Since we're on v0.113.0 now, we're in that zone where the rule does nothing. Since the right time to remove obsolete code is ASAP, now is a fine time to remove the lint from our config. :) As Greg points out, the removal of this lint rule in 0.117 should probably have been mentioned in the Flow changelog [3]. [1] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [2] zulip#4318 (comment) [3] https://github.com/facebook/flow/blob/master/Changelog.md#01170
chrisbobbe
force-pushed
the
pr-flow-pre-rn-63
branch
from
2ba4aaa
to
97c9f8d
Compare
OK, done! |
The RN v0.63 upgrade issue is #4245. There'll likely be quite a bit more work to do for Flow when we get closer to doing that upgrade, but this is a start! 🎉