Skip to content

Conversation

@thib92
Copy link
Contributor

@thib92 thib92 commented Sep 11, 2019

Summary:

Migrate packages/cli/src/commands/doctor to TypeScript, with the exported type definitions in cli-types.

Related to #683

Current progress:

  • healthchecks/**
  • doctor
  • index
  • printFixOptions
  • runAutomaticFix
  • versionRanges

Test Plan:

CI should be green

@thib92
Copy link
Contributor Author

thib92 commented Sep 11, 2019

I'm having an issue trying to move the exported types to cli-types, and I'm not sure how to resolve it:

When I try to move the types.js Flow file to the cli-types folder and export them with the cli-types package, I have some TypeScript errors regarding the version of the Node typings. I don't know how to fix this, and it's blocking me from moving the rest of the files to TS, since they all use these type definitions. If anyone has an idea on how to fix these, I'm all ears!

@thymikee
Copy link
Member

Which types you try to "move"? cc @lucasbento who's working on doctor command and may help you with some issues :)

@lucasbento
Copy link
Member

Solid work, thank you for doing this @thib92!

I would go with merging this in small chunks if possible as a lot of stuff is being changed on doctor as it's still WIP.

@thib92
Copy link
Contributor Author

thib92 commented Sep 11, 2019

@thymikee thanks for the response. I thought that cli-types was a package for all exported types. My bad.

I wanted to "move" the types.js file which contains the Flow types for the doctor command.

Therefore, moving it to types.ts sounds like a good solution.

@lucasbento sure. Now that I have an answer to my question, I'll make a minimal passing build so that it can be merged soon to allow continuous work on the command.

@thib92
Copy link
Contributor Author

thib92 commented Sep 11, 2019

@thymikee @lucasbento I think the PR is good to review as is. I moved to the types to the doctor command in the cli package as asked.

The flow check is failing now, since some JS files are trying to import files converted to TS.

Should I remove the // @flow-check mark from the files for the doctor command so that CI is green again, just for the time to migrate to TS?

@thib92 thib92 marked this pull request as ready for review September 11, 2019 22:05
@thymikee
Copy link
Member

If the file is js, leave flow alone, just add $FlowFixMe comments to the imports of TS files. See how it's done in other modules :)

@thib92
Copy link
Contributor Author

thib92 commented Sep 11, 2019

Great! That's a lot of $FlowFixMe, will need to get this moved to TS fast 😄

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. @lucasbento could you please check it as well?

Copy link
Member

@lucasbento lucasbento left a comment

Choose a reason for hiding this comment

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

This awesome ❤️

@thymikee thymikee merged commit 4b56ef4 into react-native-community:master Sep 12, 2019
@thib92
Copy link
Contributor Author

thib92 commented Sep 12, 2019

Thanks for the review @thymikee @lucasbento
I'll get started on another PR to continue the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants