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

Expose flow typings with gen-flow-files #468

Closed

Conversation

Brianzchen
Copy link
Contributor

@Brianzchen Brianzchen commented Jul 11, 2020

Fixes #165
Use https://github.com/lessmess-dev/gen-flow-files to generate flow type definitions for consuming applications

This generates tons of flow errors that can't be resolved though so users have to add the dependency to their [declarations]

[declarations]
.*node_modules/recoil/.*

Otherwise you get errors such as:
Screen Shot 2020-07-11 at 7 47 03 pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 11, 2020
@Brianzchen
Copy link
Contributor Author

Brianzchen commented Jul 11, 2020

@mondaychen This uses gen-flow-files as you recommended.

Tested and it works but I also have types-first enabled so not sure what I should do when I export my atom, probably needs more work later after the initial exposure, currently having to export as type any

// @flow
import { atom } from 'recoil';

type State = {
  text: string,
  type: NotificationType,
};

export const notificationState = (atom<State>({
  key: 'notificationState',
  default: {
    text: '',
    type: 'success',
  },
}): any);

Edit: Realised I can import type with import type { RecoilState } from 'recoil';

@mondaychen mondaychen self-assigned this Jul 12, 2020
@mondaychen
Copy link
Contributor

Thanks! I should be able to get back to you on Tuesday

@mondaychen
Copy link
Contributor

OK I think we are good with this approach.

You mentioned "This generates tons of flow errors that can't be resolved though so users have to add the dependency to their [declarations]". Is there something we can do to make this easier?

@Brianzchen
Copy link
Contributor Author

I've done further testing.

  • My initial analysis used yarn link which throws errors probably caused by the Recoil project itself.
  • But when I just copy the flow files over to my application project inside /node_modules/recoil, it throws no errors when running yarn flow and I am able to get flow type analysis in my application.
  • If I were to instead put these files in my src dir yarn flow will complain

But if I open files in node_modules there's clearly flow errors coming from my IDE though with no impact to usage,
Screen Shot 2020-07-17 at 9 02 20 am

Overall seems like this wouldn't have an impact on users and flow somehow understands that theses are declaration files from a third party and knows not to throw errors in flow analysis?

@Brianzchen
Copy link
Contributor Author

@mondaychen anymore work needs to go into this?

@Brianzchen
Copy link
Contributor Author

@drarmstr I think this PR is complete, don't think there's any more work needed, can you advise?

@mondaychen
Copy link
Contributor

Sorry @Brianzchen for being slow on this. I will create a flow-based application later this week and see how well it works. Then I'll get back to you.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mondaychen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mondaychen merged this pull request in 4a25ce6.

@mondaychen mondaychen mentioned this pull request Aug 22, 2020
@sherwinyu
Copy link

Hello, I'm trying to get flow types working for React but I still encounter Cannot resolve module recoil. when typechecking with flow. I'm using recoil v0.4.1.

Any guidance on how to actually get flow to pick up these types? Do I need to do anything else with .flowconfig?

@Brianzchen Brianzchen deleted the gen-flow-files branch October 3, 2021 01:54
@Brianzchen
Copy link
Contributor Author

Any guidance on how to actually get flow to pick up these types? Do I need to do anything else with .flowconfig?

Looks like recoil still ships with flow types. What does your flowconfig look like? Best to make sure you're not excluding recoil in your ignore group.

AlexGuz23 pushed a commit to AlexGuz23/Recoil that referenced this pull request Nov 3, 2022
Summary:
Fixes facebookexperimental/Recoil#165
Use https://github.com/lessmess-dev/gen-flow-files to generate flow type definitions for consuming applications

This generates tons of flow errors that can't be resolved though so users have to add the dependency to their `[declarations]`

```
[declarations]
.*node_modules/recoil/.*
```

Otherwise you get errors such as:
<img width="908" alt="Screen Shot 2020-07-11 at 7 47 03 pm" src="https://user-images.githubusercontent.com/12436524/87221437-53bfe700-c3af-11ea-9ab5-31a1c7c82d40.png">

Pull Request resolved: facebookexperimental/Recoil#468

Reviewed By: drarmstr

Differential Revision: D23144185

Pulled By: mondaychen

fbshipit-source-id: 7359ac1e75b29cfa24ef516f1a3d7ed0221d3a29
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this pull request Mar 5, 2023
Summary:
Fixes facebookexperimental/Recoil#165
Use https://github.com/lessmess-dev/gen-flow-files to generate flow type definitions for consuming applications

This generates tons of flow errors that can't be resolved though so users have to add the dependency to their `[declarations]`

```
[declarations]
.*node_modules/recoil/.*
```

Otherwise you get errors such as:
<img width="908" alt="Screen Shot 2020-07-11 at 7 47 03 pm" src="https://user-images.githubusercontent.com/12436524/87221437-53bfe700-c3af-11ea-9ab5-31a1c7c82d40.png">

Pull Request resolved: facebookexperimental/Recoil#468

Reviewed By: drarmstr

Differential Revision: D23144185

Pulled By: mondaychen

fbshipit-source-id: 7359ac1e75b29cfa24ef516f1a3d7ed0221d3a29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build / infra CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flow support
5 participants