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

Flow to TypeScript conversion #560

Merged
merged 8 commits into from
Dec 16, 2021
Merged

Conversation

saulipurhonen
Copy link
Contributor

@saulipurhonen saulipurhonen commented Dec 3, 2021

Description

Project wide conversion from Flow to TypeScript.

Related issues

Closes #448

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

  • TS configurations (in eslintrc.json and tsconfig.json)
  • ESLint configurations with TS
  • Converted files to .ts and .tsx extensions
  • Installed necessary types
  • Use redux useSelector and useDispatch as custom typed methods (useAppSelector, useAppDispatch)
  • Module augmentations for MUI Theme and Palette to enable custom properties
  • Improved types throughout project tree
  • Removed Flow related configurations, including workflow for static types check. This is handled via eslint
  • Cypress TS configuration
  • Introduced types for custom Cypress commands
  • Updated README.md
  • Introduced TSC check in Husky pre-commit hook and GitHub Actions workflow

Testing

  • Tests do not apply

@saulipurhonen saulipurhonen force-pushed the feature/flow-to-ts-conversion branch 7 times, most recently from 99b7b3c to b7ba56b Compare December 3, 2021 07:41
@saulipurhonen saulipurhonen self-assigned this Dec 3, 2021
@saulipurhonen saulipurhonen added the enhancement New feature or request label Dec 3, 2021
@saulipurhonen saulipurhonen added this to In progress in SD Submit via automation Dec 3, 2021
@saulipurhonen saulipurhonen added this to the Open Beta milestone Dec 3, 2021
@saulipurhonen saulipurhonen marked this pull request as ready for review December 3, 2021 08:08
Copy link
Contributor

@blankdots blankdots left a 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 ?

@saulipurhonen
Copy link
Contributor Author

saulipurhonen commented Dec 3, 2021

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 workflow pre-commit hook and GH action workflow into this PR.

@saulipurhonen
Copy link
Contributor Author

saulipurhonen commented Dec 3, 2021

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 workflow pre-commit hook and GH action workflow into this PR.

Husky pre-commit hook and GH Actions workflow now introduced.

SD Submit automation moved this from In progress to Reviewer approved Dec 7, 2021
Copy link
Contributor

@hannyle hannyle left a 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 ?

@saulipurhonen
Copy link
Contributor Author

saulipurhonen commented Dec 10, 2021

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 react-hooks/exhaustive-deps eslint rule. In recommended eslint configuration we'd have this rule to warn on improperly used hooks. See Hooks rules.

I had this rule configured at the beginning of conversion (and eslint suggested fix on adding dispatch as second argument for useEffect hook in this case) but I decided to leave it off at the moment to help finish the conversion faster.

Edit: Added link to hooks rules

Copy link
Contributor

@blankdots blankdots left a 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.

.github/workflows/typecheck.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@saulipurhonen
Copy link
Contributor Author

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 no-explicit-any and type all instances of any. This however can be time consuming and I suggest this to be done on different PR.

@blankdots
Copy link
Contributor

This was on purpose to save time. I think we could introduce eslint rule no-explicit-any and type all instances of any. This however can be time consuming and I suggest this to be done on different PR.

that what is what i implied as well.

also sry did not realised me merging the dependencies cause package lock to get conflicts

@saulipurhonen
Copy link
Contributor Author

also sry did not realised me merging the dependencies cause package lock to get conflicts

No prob.

@blankdots
Copy link
Contributor

blankdots commented Dec 10, 2021

I continued on this PR as established with @saulipurhonen in RC

@blankdots blankdots force-pushed the feature/flow-to-ts-conversion branch 8 times, most recently from b8eccee to 96ae9cb Compare December 10, 2021 15:25
@blankdots
Copy link
Contributor

blankdots commented Dec 10, 2021

seems pressing the Submit Study button does not work for some reason 🤷‍♂️

FormSelectField seem to be broken

src/components/Nav.tsx Outdated Show resolved Hide resolved
@hannyle
Copy link
Contributor

hannyle commented Dec 16, 2021

seems pressing the Submit Study button does not work for some reason 🤷‍♂️

FormSelectField seem to be broken

--> Fixed for FormSelectField, the function responsible for changing fieldValue was broken in this branch

The PR is in quite a good shape now and can be merged. There are still some issues left as mentioned: types as any, and enable eslint rule react-hooks/exhaustive-deps again, but I think this could be done separately in another PR.

@blankdots
Copy link
Contributor

then I will click merge and we can see what happens.

@blankdots blankdots merged commit b7ec0fe into develop Dec 16, 2021
SD Submit automation moved this from Reviewer approved to Done Dec 16, 2021
@blankdots blankdots deleted the feature/flow-to-ts-conversion branch December 16, 2021 06:31
@blankdots blankdots mentioned this pull request Apr 7, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
SD Submit
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants