-
Notifications
You must be signed in to change notification settings - Fork 2
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
Flow to TypeScript conversion #560
Conversation
99b7b3c
to
b7ba56b
Compare
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.
a comment while i review this, is it possible or useful to use the tsc (https://www.typescriptlang.org/docs/handbook/compiler-options.html) to do typechecks e.g. https://github.com/typescript-eslint/typescript-eslint/blob/main/package.json#L48 so that we can implement that as part of a github action ?
I think this a great idea and in addition should be added to Husky pre-commit hooks. I'll try to suggest both |
Husky pre-commit hook and GH Actions workflow now introduced. |
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.
Looks very good to me!
I still have a question: Why do we need to put dispatch
as [dispatch]
in useEffect
, for e.g. in App.tsx
, instead of [ ]
as usual ? Any documents regarding to this ?
Good call. Empty second arguments on useEffect Hook is against I had this rule configured at the beginning of conversion (and eslint suggested fix on adding Edit: Added link to hooks rules |
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.
there are a lot of types as any
, but I assume we can revisit that.
This was on purpose to save time. I think we could introduce eslint rule |
that what is what i implied as well. also sry did not realised me merging the dependencies cause package lock to get conflicts |
No prob. |
6b2db29
to
25835ea
Compare
I continued on this PR as established with @saulipurhonen in RC |
b8eccee
to
96ae9cb
Compare
96ae9cb
to
98ef5bd
Compare
|
--> Fixed for The PR is in quite a good shape now and can be merged. There are still some issues left as mentioned: types as |
then I will click merge and we can see what happens. |
Description
Project wide conversion from Flow to TypeScript.
Related issues
Closes #448
Type of change
Changes Made
Testing