-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Removed Flow #2385
Removed Flow #2385
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2fd71a9:
|
That's a good question - I think we can remove them since atm we target the |
Codecov Report
|
"import/no-duplicates": 0, | ||
"prettier/prettier": [ | ||
"error", | ||
{ | ||
"parser": "flow" | ||
"parser": "typescript-eslint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting - I would have assumed that javascript/typescript/babel-ts parsers would work here. I understand the difference between them but I don't immediately see why there is a need for Prettier's typescript-eslint to exist in the first place. Is this just about making sure what both use the same baseline of features? So if something can be parsed using typescrip-teslint then it can also be parsed using Prettier? I suspect that otherwise we could end up in a situation with a single file being parseable by one without being parseable by the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they're pretty equivalent. For now it's all vanilla JavaScript, which TypeScript supports. I'd think long-term it'd be easier to just have one parser for all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have mentioned that:
$FlowFixMes are removed; only one of their comments is kept for not being related to the Flow type checker
I believe that you are referring to the comment in packages/react/__tests__/theme-provider.js
- I can't find a reason why this would be kept. I'm not sure why it's there but after this PR lands, it will be completely obsolete anyway.
Excellent work 🚀 I love how you have left the flow types as comments for now. As soon as this PR gets ✅ we'll be able to merge.
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
* Add codecademy.com to "in the wild" list We use Emotion! And I would like to run builds for #2385 😄 * Update README.md
What:
Removes all Flow tooling from source code.
*
and<empty>
, are removed altogether.$FlowFixMe
s are removed; only one of their comments is kept for not being related to the Flow type checkerWhy:
Following the strategy mentioned by @Andarist in #2358 (comment).
How:
I did a find-all for strings such as
:
andComponent<
, then continuously ranyarn lint
,yarn build
, andyarn test
until there were no more errors. Alsoyarn test -u
to clean up the auto-generated snapshots.Checklist:
Question: This also removes the Flow docs page, because Flow types are no longer generated. Is that right for this stage? My guess is that they could be generated from TypeScript types later?