react-nav libdefs: Move bottom-tabs to its own .flow.js file #5233

chrisbobbe · 2022-02-15T04:06:52Z

(And, in the first commit, a type-level bugfix cherry-picked from #5197.)

Inspired by 007dea3.

I'd like to do this for all the libdefs currently in flow-typed/@react-navigation. That should let us deduplicate a lot of code, and make it easier to update the definitions when we move to React Navigation 6.

I'm not fussy where the test file ends up; I just noticed that ad82c4c expressed a wish for it to be closer to the code being tested, but there may be a nicer solution than this one. 🙁

gnprice

Thanks! Small comments below, then please merge at will. Excited to see that this seems quite tractable to do.

gnprice · 2022-02-17T00:55:51Z

flow-typed/@react-navigation/drawer_v5.x.x.js

  declare export type ScreenListeners<
-    EventMap: EventMapBase = EventMapCore<State>,
    State: NavigationState = NavigationState,
+    EventMap: EventMapBase = EventMapCore<State>,


Hmm, good fix!

The error shows up when we move this to a `.flow.js` file, a pattern Greg started in 007dea350. Presumably it doesn't show up now because Flow is just trying to gather exported types from files in the flow-typed/ directory, not check exhaustively for errors in those files.

I think in fact the story is that Flow really was trying to check for errors in these files, but there just was a bug. The bug was a very bad one if you were trying to use libdef files widely (à la flow-typed), but upstream only uses libdefs for a narrow set of use cases like builtins, so the bug went undetected for years.

Here's the bugfix:
facebook/flow@ca7d6ef

Those "previous couple of diffs" it mentions are these:
facebook/flow@0d997b6
facebook/flow@b5f9473
(which you can also find with git log --stat -p -3 --author pvekris ca7d6efb2e7c45bbf26090a729892d9c29861159 in a flow.git worktree)

(Further evidence that upstream does not much use libdefs is the absolutely catastrophic bug -- catastrophic if you were trying to use libdefs, that is -- fixed in this commit a bit later toward v0.143.0: facebook/flow@d35f6dc That one had been reported at least 11 different times starting in 2017, with nobody in those threads figuring out what was actually happening until the bug had been fixed.)

gnprice · 2022-02-17T01:10:33Z

flow-typed/@react-navigation/material-top-tabs_v5.x.x.js

-        empty,
-        $Shape<$NonMaybeType<$ElementType<ParamList, RouteName>>>,
-      >,
+      // We've edited this to be less complicated, so Flow in types-first


We should really deduplicate this stuff; we've found out that this should be possible with the .js.flow strategy Greg started using in 007dea350.

Wow, I see, these files really have a ton in common!

They're all over 2000 lines long:

$ git ls flow-typed/ | xargs wc -l … 2101 flow-typed/@react-navigation/drawer_v5.x.x.js 2046 flow-typed/@react-navigation/material-top-tabs_v5.x.x.js 2185 flow-typed/@react-navigation/native_v5.x.x.js 2137 flow-typed/@react-navigation/stack_v5.x.x.js

But the diffs are much smaller than that:

$ git di --stat upstream:flow-typed/@react-navigation/{material-top,bottom}-tabs_v5.x.x.js ...rial-top-tabs_v5.x.x.js => bottom-tabs_v5.x.x.js} | 38 ++++++++++------------ 1 file changed, 18 insertions(+), 20 deletions(-) $ git di --stat upstream:flow-typed/@react-navigation/{bottom-tabs,stack}_v5.x.x.js .../{bottom-tabs_v5.x.x.js => stack_v5.x.x.js} | 141 +++++++++++++++++---- 1 file changed, 118 insertions(+), 23 deletions(-)

(and similarly the others.)

gnprice · 2022-02-17T01:15:17Z

jest.config.js

+      '/node_modules/',
+      '/src/__tests__/lib/',
+      '-testlib.js$',
+      '/types/__tests__',


How about putting the tests under types/__flow-tests__/ instead? Like our existing src/__flow-tests__/.

Then the name won't look in the first place like it's something Jest should be running, so this line shouldn't be needed. Also the eslintignore line won't be needed, because there's already one for **/__flow-tests__/**.

Also the eslintignore line won't be needed, because there's already one for **/__flow-tests__/**.

Hmm I'm not sure which line this is referring to.

Ah, no, I was misreading re: eslintignore. Still happy with the new name, though.

The `State` in `EventMapCore<State>` was surely meant to refer to `ScreenListeners`'s type parameter, not something defined at toplevel in the module. (There isn't a definition at toplevel.) I believe `State` refers to the shape of our app's own custom navigation state, so it needs to be something the caller provides: https://reactnavigation.org/docs/5.x/navigation-events Thankfully, this small reorder is enough to bring `State` into scope, and there aren't many callers. We do end up repeating the change across all five of our react-nav libdefs. The error has been swallowed because Flow has been bad at catching errors in libdef files; see Greg's comment at zulip#5233 (comment) .

chrisbobbe · 2022-02-18T23:48:29Z

Thanks for the review! Revision pushed, with a reply at #5233 (comment).

…ibdefs Doing the same as we did for the stack-nav libdef in 49c7eef and ad82c4c. We should really deduplicate this stuff; we've found out that this should be possible with the .js.flow strategy Greg started using in 007dea3.

The `State` in `EventMapCore<State>` was surely meant to refer to `ScreenListeners`'s type parameter, not something defined at toplevel in the module. (There isn't a definition at toplevel.) I believe `State` refers to the shape of our app's own custom navigation state, so it needs to be something the caller provides: https://reactnavigation.org/docs/5.x/navigation-events Thankfully, this small reorder is enough to bring `State` into scope, and there aren't many callers. We do end up repeating the change across all five of our react-nav libdefs. The error has been swallowed because Flow has been bad at catching errors in libdef files; see Greg's comment at zulip#5233 (comment) .

…diff To see that the whitespace is the only change, view the diff with git diff -w

Inspired by 007dea3. I took everything in the `declare module` block (thankfully there was just one such block), moved it to the new file, and removed `declare` everywhere, as in `declare export type …`. And move the tests closer to the file, in `types/__flow-tests__/`. When we started adding these test files, in ad82c4c, we said it'd be best for the tests to live closer to the code being tested.

gnprice · 2022-02-19T00:36:07Z

Thanks! Looks good -- merging.

chrisbobbe requested a review from gnprice February 15, 2022 04:06

chrisbobbe force-pushed the pr-react-nav-libdefs branch from 89ee435 to 46437f9 Compare February 15, 2022 04:10

chrisbobbe mentioned this pull request Feb 16, 2022

Upgrade to RN v0.65 #5230

Closed

gnprice reviewed Feb 17, 2022

View reviewed changes

chrisbobbe force-pushed the pr-react-nav-libdefs branch from 46437f9 to b09a4a8 Compare February 18, 2022 23:48

chrisbobbe added 7 commits February 18, 2022 16:34

react-nav libdefs: Replace unclear types with $FlowFixMe

53838ee

react-nav libdefs: Fix spelling error in a type alias

4b02f8f

react-nav libdefs [nfc]: TODOs for filling in some types in bottom-tabs

0331f40

react-nav libdefs [nfc]: Decrease indent in bottom-tabs, to simplify …

314d089

…diff To see that the whitespace is the only change, view the diff with git diff -w

gnprice force-pushed the pr-react-nav-libdefs branch from b09a4a8 to 76688d5 Compare February 19, 2022 00:36

gnprice merged commit 76688d5 into zulip:main Feb 19, 2022

chrisbobbe deleted the pr-react-nav-libdefs branch February 19, 2022 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

react-nav libdefs: Move bottom-tabs to its own .flow.js file #5233

react-nav libdefs: Move bottom-tabs to its own .flow.js file #5233

Uh oh!

chrisbobbe commented Feb 15, 2022 •

edited

Loading

Uh oh!

gnprice left a comment

Uh oh!

gnprice Feb 17, 2022

Uh oh!

gnprice Feb 17, 2022

Uh oh!

gnprice Feb 17, 2022

Uh oh!

chrisbobbe Feb 18, 2022

Uh oh!

gnprice Feb 19, 2022

Uh oh!

chrisbobbe commented Feb 18, 2022

Uh oh!

gnprice commented Feb 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

react-nav libdefs: Move bottom-tabs to its own .flow.js file #5233

react-nav libdefs: Move bottom-tabs to its own .flow.js file #5233

Uh oh!

Conversation

chrisbobbe commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Feb 18, 2022

Uh oh!

gnprice commented Feb 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe commented Feb 15, 2022 •

edited

Loading