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

Add flow types to flowtype/flow-typed #3146

Closed
maxh opened this issue Dec 18, 2017 · 10 comments

Comments

@maxh
Copy link

@maxh maxh commented Dec 18, 2017

I ran into some flow issues. I filed facebook/flow#5483 against the flow team asking them to make it easier to isolate individual files to be flow checked from the react-navigation repo:

facebook/flow#5483

Their suggestion is:

"Instead, react-navigation should include it's type definitions in the flowtype/flow-typed repository, where they can be versioned by the library version and by Flow version."

https://github.com/flowtype/flow-typed

Just filing this as a FR here -- thanks!

@kelset

This comment has been minimized.

Copy link
Contributor

@kelset kelset commented Dec 19, 2017

cc @Ashoat

@Ashoat

This comment has been minimized.

Copy link
Member

@Ashoat Ashoat commented Dec 19, 2017

Yeah, we’re aware. Four thoughts:

  1. You actually shouldn’t be seeing Flow errors if you’re using react-navigation correctly with recent versions of react-native. What errors are you seeing exactly?
  2. Almost all uses of react-navigation are coupled with react-native, and since react-native leaves types in the library instead of publishing to flow-typed, many of the advantages for us to publish to flow-typed are nullified (eg. enabling users to use more than one Flow version with our library).
  3. As this library is still in beta, our types are fairly unstable, and we change them regularly. Maintaining flow-typed definitions is quite a bit of work (for instance, test files need to be created), and means any time we change types we need to do some additional work to maintain the flow-typed side.
  4. I am the main person supporting Flow in react-navigation right now and I don’t have the spare cycles to maintain flow-typed libdefs at this time. Considering all the above, if you’d like to volunteer for it (on a continuous basis) then perhaps we can make it happen!

For now I’m gonna close this issue, but I welcome continuing discussion, and I think our answer to this will very possibly change in the future (if and when react-native starts publishing flow-typed libdefs and we come out of beta).

@Ashoat Ashoat closed this Dec 19, 2017
@maxh

This comment has been minimized.

Copy link
Author

@maxh maxh commented Dec 20, 2017

Thanks for the quick reply -- totally understand. Next time I encounter flow errors I'll report them here!

@PiTiLeZarD

This comment has been minimized.

Copy link

@PiTiLeZarD PiTiLeZarD commented Jan 2, 2018

Hey,

I ended up here after googling the sh**t out of flow, flow-typed, react-navigate and I eventually looked for a type definition for navigate so here I am. I comment here since it's been updated fairly recently, my apologies if it's not the right place to do so.

My experience:

  • Create an expo/yarn react-native project
  • Use my own pretty limited navigation thingy, pat myself in the back a few times
  • Look for better, stuck with expo and all so no native solution, I'm going for react-navigation which seems pretty good
  • yarn add react-navigation
  • yarn flow -> Module not found (whereas it's right there, I can see it, if only I could show you!!! :) )
  • install flow-typed and shield react-navigate (I dont get the error but my experience makes me really unhappy about this solution)

Now I'm having issues with partially typed States and Props which are impossible to solve.

The weird thing is:
8: import type {NavigationScreenProp} from "react-navigation/src/TypeDefinition";
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ react-navigation/src/TypeDefinition. Required module not found

Which also doesn't work with relative imports. So it seems like flow and react-navigate had a fight, and I'm not good at couple counselling.

About versions: npm ls --depth=0 gives me:
├── expo@23.0.6
├── flow-bin@0.56.0
├── jest-expo@23.0.0
├── node-pre-gyp@0.6.39 extraneous
├── react@16.0.0
├── UNMET PEER DEPENDENCY react-native@0.50.3
├── react-native-elements@0.18.5
├── react-native-maps@0.19.0
├── react-native-scripts@1.8.1
├── react-native-svg@5.5.1 extraneous
├── react-navigation@1.0.0-beta.22
├── react-test-renderer@16.0.0
└── websql@0.4.4 extraneous

The unmet dependency is apparently due to react-native-maps who's looking for react-native 0.51, I didn't bother since it works fine as is. And I use yarn v0.27.5, node v8.4.0, dont know what else could be useful?

Any help on this would be gratefully received.

Cheers

@Ashoat

This comment has been minimized.

Copy link
Member

@Ashoat Ashoat commented Jan 3, 2018

I don’t know why that’s happening. Something must be incorrectly configured, since it should be possible to import directly from that file. I’m guessing something is wrong with your .flowconfig. I’ve only tested .flowconfig from React Native projects (react-native init gives you one).

You should be able to import directly from react-navigation instead of TypeDefinition, by the way.

@PiTiLeZarD

This comment has been minimized.

Copy link

@PiTiLeZarD PiTiLeZarD commented Jan 8, 2018

Finally got it to work... So:

  • react-navigation was already being ignored in the flowconfig so I removed it (didn't add it myself so I assumed it was not)
  • added module.file_ext=.native.js in the [options] section
  • added <PROJECT_ROOT>/node_modules/react-navigation/./tests/. in the [ignore] section

now I have the "No errors" message and I can import types! Very happy with this, thanks for getting back to me on this one!

@quincycs

This comment has been minimized.

Copy link

@quincycs quincycs commented Jan 8, 2018

@PiTiLeZarD thanks! I had this problem too, this worked for me.

@Ashoat Ashoat reopened this Jan 22, 2018
@Ashoat

This comment has been minimized.

Copy link
Member

@Ashoat Ashoat commented Jan 22, 2018

In talking to @satya164 I realized that reason #2 above is not actually true. Since Flow versions correspond to React Native versions, we can upload types for each individual Flow/RN version to flow-typed. That way, users of the library can use any version of RN that they want, download the corresponding types via flow-typed, and things should typecheck.

I've reopened this issue to track. I don't have the cycles right now to do this, but might in the next couple of months. If anybody is interested in addressing this, please let me know!

@brentvatne

This comment has been minimized.

@brentvatne brentvatne closed this Jan 23, 2018
@maxh

This comment has been minimized.

Copy link
Author

@maxh maxh commented Mar 19, 2018

just confirming that we're now using flow-typed for react navigation and it's great! thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.