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

fix(typescript): Use consistent typescript configs - N8N-4552 #4049

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

netroy
Copy link
Member

@netroy netroy commented Sep 7, 2022

make all packages inherit tsconfig from the root tsconfig.

Ticket: N8N-4537

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request ui Enhancement in /editor-ui or /design-system labels Sep 7, 2022
@netroy netroy removed the request for review from agentreno September 7, 2022 08:12
@netroy netroy changed the title fix(typescript): Use consistent typescript configs fix(typescript): Use consistent typescript configs - N8N-4552 Sep 7, 2022
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

Builds fine, runs fine, only minor comments.

  1. Let's remove all tsx references? "src/**/*.tsx" "tests/**/*.tsx",, etc.

  2. useUnknownInCatchVariables Comment to enable in future?

  3. Settings like strictNullChecks, noImplicitAny, etc. are part of strict, which is enabled everywhere except cli. Maybe a comment to remove these settings that are part of strict once we have strict all over?

  4. If we only need emitDecoratorMetadata and experimentalDecorators for TypeORM and for your router refactorings in cli, should we set them there only?

  5. "outDir": "dist" is repeated in all packages. This is not inheritable?

Ideally if @alexgrozav can check the tsconfig for the FE packages.

@netroy netroy force-pushed the unify-tsconfig branch 3 times, most recently from 0ec10d5 to 4047c67 Compare September 8, 2022 17:47
@netroy
Copy link
Member Author

netroy commented Sep 8, 2022

Builds fine, runs fine, only minor comments.

1. Let's remove all tsx references? `"src/**/*.tsx"` `"tests/**/*.tsx",`, etc.

Done ✔️

2. `useUnknownInCatchVariables` Comment to enable in future?

enabled it globally, and disabled it on projects that needed it. So now the flag is under a TODO now.

3. Settings like `strictNullChecks`, `noImplicitAny`, etc. are part of `strict`, which is enabled everywhere except cli. Maybe a comment to remove these settings that are part of `strict` once we have `strict` all over?

I tried moving those to the cli tsconfig, but couldn't get it to work. I'll prioritize switching on strict for cli. we can remove these then.

4. If we only need `emitDecoratorMetadata` and `experimentalDecorators` for TypeORM and for your router refactorings in cli, should we set them there only?

Done ✔️

5. `"outDir": "dist"` is repeated in all packages. This is not inheritable?

unfortunately not.
From what I understand, outDir is relative to the tsconfig defining it.

ivov
ivov previously approved these changes Sep 9, 2022
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

Rebuilt, retested, all good.

  1. Do we need to build the tests? If I remove them from tsconfig, the test command runs fine, maybe ts-jest transpiles them on the fly.

  2. When saving the tsconfigs locally, they are getting autoformatted with Prettier. If you can please format them and set up the extension dangmai.workspace-default-settings so your editor uses .vscode/settings.default.json to do this automatically in future.

Let's wait for Alex but otherwise LGTM. Thanks for cleaning this up.

@netroy
Copy link
Member Author

netroy commented Sep 9, 2022

Rebuilt, retested, all good.

6. Do we need to build the tests? If I remove them from tsconfig, the test command runs fine, maybe ts-jest transpiles them on the fly.

exactly, ts-jest transpiles the tests. we don't need to compile them.

7. When saving the tsconfigs locally, they are getting autoformatted with Prettier. If you can please format them and set up the extension `dangmai.workspace-default-settings` so your editor uses `.vscode/settings.default.json` to do this automatically in future.

I'll send another PR where I enable prettier & prettier-eslint on everything. Will reformat all JSON files in there as well.

Let's wait for Alex but otherwise LGTM. Thanks for cleaning this up.

@ivov
Copy link
Contributor

ivov commented Sep 9, 2022

we don't need to compile them.

Perfect, let's remove them all then from tsconfigs, to reduce build time and published package size.

@netroy
Copy link
Member Author

netroy commented Sep 9, 2022

we don't need to compile them.

Perfect, let's remove them all then from tsconfigs, to reduce build time and published package size.

done. also, ran prettier on all the tsconfigs.

@netroy netroy requested a review from ivov September 9, 2022 08:55
ivov
ivov previously approved these changes Sep 9, 2022
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

🚀

BHesseldieck
BHesseldieck previously approved these changes Sep 9, 2022
make all packages inherit tsconfig from the root tsconfig
skips building tests. reformat all tsconfigs with prettier.
@netroy netroy requested a review from ivov September 9, 2022 15:59
@netroy netroy merged commit 9267e8f into n8n-io:master Sep 9, 2022
@netroy netroy deleted the unify-tsconfig branch September 9, 2022 16:00
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Sep 9, 2022
@janober
Copy link
Member

janober commented Sep 15, 2022

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Sep 15, 2022
valya pushed a commit to valya/n8n that referenced this pull request Nov 8, 2022
…#4049)

fix(typescript): Use consistent typescript configs

make all packages inherit tsconfig from the root tsconfig
skips building tests. reformat all tsconfigs with prettier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants