-
Notifications
You must be signed in to change notification settings - Fork 520
Disable js typechecking by default and adjust tsconfig to be more performant and reduce common errors by configuration less strict #5186
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
base: develop
Are you sure you want to change the base?
Conversation
|
This Pull Request targets Consider targeting |
|
Discussed in HOC: We don't want to overwhelm our devs yet with all the errors that would be thrown. I'll try to come up with a minimal starting tsconfig which should solve most of our type errors but not be as strict as this one |
|
@Buckwich If you need help, please reach out. And thanks for fixing the config 🙂. Apparently it works in my editor, independent of the |
|
I tried to reduce the error count as much as possible but its still 1326 errors in 227 files. At least i can run tsc now without running out of memory. Based on our discussion in HOC I would suggest that those that don't want to get all those errors in their IDE, overwrite the tsconfig and add it to their global git ignore (alternative would be to do it the other way around) |
| "src/app/*": [ "./client/src/app/*" ], | ||
| "test/*": [ "./client/test/*" ] | ||
| }, | ||
| "types": [ |
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.
Why do we need this in addition to adding the @types/* dependency?
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.
type acquisition does not work for types specified in the tsconfig itself. As we dont import describe/it/expect this tells which types to use for them, basicly all umdglobal imports.
Alternative would be to always import describe/it/expect in all test files.
| "noEmit": true, | ||
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "allowUmdGlobalAccess": true, |
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.
What do we need this for?
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.
| "strictNullChecks": false, | ||
|
|
||
| }, | ||
| "typeAcquisition": { |
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.
Why do we need this?
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.
without it we would need to add a lot more @types packages, at the moment i only have added the test types as they can't be auto acquired. This is also an IDE setting so we could remove it from the default config
| "app", | ||
| "globals.d.ts" | ||
| ], | ||
| "exclude": [ |
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.
This is a performance enhancement, to prevent out of memory?
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.
yes, i had issues running npx tsc without it, also vscode complains about too many files to check without it. Because we have nested node_modules/build the default configs don't work
|
@Buckwich do we actually want to merge it soon or does it require more discussion? |
f1ffdb2 to
6d832c1
Compare
|
It can be merged if it is approved and accepted by the team that we will get more errors in the code. I'm already using it, its helpful but the errors can be distracting. And we have to accept that we can't fix all errors if we are in a js project |
I kinda don't like that and would prefer to work in a setup where each error is meaningful. 🙁 |
6d832c1 to
76e7997
Compare
|
I've split the PR, this one contains only the tsconfig adjustments, type fixes are in #5251. Now we don't ship with a default tsconfig (same behavior as previous / before 5bd8d9e). but have a simple way to enable it for those who like it: Copy |
|
From today's Hour of Code: Consider having one |


Proposed Changes
In 5bd8d9e a tsconfig was added with invalid keys, this PR fixes that but I want to make the team aware of the implications.
To remove some errors i also added jsx parsing and other configurations to make tsc behave as much as our js codebase as possible. (its still not perfect)
With https://www.typescriptlang.org/tsconfig/#checkJs a lot of ts error will be shown in vscode, due to the strict config also a lot of complaints of implicit any

Until we have improved our type safety I would suggest to be not as strict. Disabling the following strict checks:
As discussed in HOC we don't want to force a wave of errors to our developers and make it an opt-in option. With this PR we don't ship with a default tsconfig (same behavior as previous / before 5bd8d9e). but have a simple way to enable it for those who like it: Copy
tsconfig.enable.jsontotsconfig.json(which is now gitignored)Try out
Copy
tsconfig.enable.jsontotsconfig.jsonOpen a file in an IDE with TS support. To get a full list of errors just run tsc, its configured to not emit:
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}