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

added typechecking for all ts files #6466

Merged
merged 13 commits into from
Aug 20, 2024

Conversation

Mihan786Chistie
Copy link
Contributor

@Mihan786Chistie Mihan786Chistie commented Jul 31, 2024

Fixes: #6436

Changes made:

  • Added typecheck step before twenty-ui build to check stories TS errors

  • Added a tsconfig.dev.json to add stories and tests to typecheking when in dev mode

  • Added tsconfig.dev.json to storybook dev command of twenty-ui to typecheck stories while developing

  • Fixed twenty-ui stories that were broken

  • Added a serve command to serve front build

  • Fixed unit test from another PR

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request introduces type-checking for all TypeScript files, including test and story files, to improve development workflow and catch errors early.

  • Updated packages/twenty-front/tsconfig.json to include all .ts, .tsx, .stories.ts, .stories.tsx, .test.ts, and .test.tsx files.
  • Modified CI configuration to run npx tsc --noEmit for type-checking.
  • Configured type-checking to execute only when matrix.task is set to typecheck.

These changes ensure comprehensive type-checking coverage, potentially increasing type-checking time but significantly improving error detection during development.

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

Comment on lines 31 to 37
"include": [
"src/**/*.ts",
"src/**/*.tsx",
"src/**/*.stories.ts",
"src/**/*.stories.tsx",
"src/**/*.test.ts",
"src/**/*.test.tsx"
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Including all test and story files in type-checking might increase the CI build time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the solution would involve tsconfig.app.json, maybe we should just remove the excluded array ?

Please test by modifying a TS type that is both present in a .tsx file and a story file, you dev env in VS Code should return an error for the story, while it is not doing it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will try that

tasks: ${{ matrix.task }}
- name: TypeScript Type Check
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not necessary we already have a CI task for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Comment on lines 31 to 37
"include": [
"src/**/*.ts",
"src/**/*.tsx",
"src/**/*.stories.ts",
"src/**/*.stories.tsx",
"src/**/*.test.ts",
"src/**/*.test.tsx"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the solution would involve tsconfig.app.json, maybe we should just remove the excluded array ?

Please test by modifying a TS type that is both present in a .tsx file and a story file, you dev env in VS Code should return an error for the story, while it is not doing it right now.

@Mihan786Chistie
Copy link
Contributor Author

@lucasbordeau Please check the recent fixes

@Mihan786Chistie
Copy link
Contributor Author

I was able to get the errors after changing the types in the files:

.tsx file:
20error1

.stories.tsx file:
20error2

@Mihan786Chistie
Copy link
Contributor Author

@lucasbordeau Any updates?

@FelixMalfait
Copy link
Member

Thanks!
I was about to merge this but I realize this might build tests/stories into the prod app (?).
One solution would be to do environment-specific tsconfig.json (possible to avoid copy/paste through inheritance)
Let's wait for Lucas to decide when he comes back next week

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Aug 12, 2024

@Mihan786Chistie Félix is right, let's create a new tsconfig.dev.json, where we'll put the config that includes everything.

Also rename the app.json => prod.json

We should then use the variable isBuildCommand in vite config to determine which tsconfig we should use.

Please check that both the dev tsconfig still works with the same test and that the build also works and doesn't include the stories and spec files. (check in the outDir javascript files)

@Mihan786Chistie
Copy link
Contributor Author

@Mihan786Chistie Félix is right, let's create a new tsconfig.dev.json, where we'll put the config that includes everything.

Also rename the app.json => prod.json

We should then use the variable isBuildCommand in vite config to determine which tsconfig we should use.

Please check that both the dev tsconfig still works with the same test and that the build also works and doesn't include the stories and spec files. (check in the outDir javascript files)

Noted, will make the changes accordingly

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Aug 19, 2024

@Mihan786Chistie I've made the required changes, there's only twenty-ui stories that are still left unchecked by twenty-front dev command, I'll fix it and it'll be done.

It was a bit tricky with our configs, so I made it, we'll find another good first issue for you !

@Mihan786Chistie
Copy link
Contributor Author

@lucasbordeau Thanks for the help. Will look forward for other issues

@lucasbordeau lucasbordeau merged commit be20a69 into twentyhq:main Aug 20, 2024
9 of 11 checks passed
@lucasbordeau
Copy link
Contributor

@Mihan786Chistie I'm assigning you on this issue : #4871

@Mihan786Chistie Mihan786Chistie deleted the typecheck-ts-files branch August 20, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add all TS files to front type checker.
4 participants