-
Notifications
You must be signed in to change notification settings - Fork 345
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
feat: Ensure strict(er) TS compliance for the generated code #868
Conversation
- Add a new `tsc:check` script to ensure that there are no TS files with undetected errors. - Split the TS configs in `integrations` so the compiled proto TS files can underly much stricter configuration requirements than the test files themselves. - Add `DOM` to `compilerOptions.lib` for integration tests to have DOM's `AbortSignal` in scope. - Apply the strictest TS preset there is to the compiled proto TS files (with a few exceptions that may be resolved later). - Update the generated code to adhere to the stricter TS config. - Modernise a few areas of the generated code. - Add `tsc:check` to the CI script
Missed one test apparently, fixed. I noticed that the CI still runs Node 14. Considering it is EOL and I've bumped the Node types, should it be removed from the test matrix? |
@threema-lenny sure, I'm good dropping Node 14 support, thanks! |
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.
Hi @threema-lenny , this looks really great, thanks for catching these issues and the clean up! I'm still going through the PR and might have another question or two, hopefully should review the rest of it tomorrow. Thanks!
I've removed Node 14 and added Node 20 in the CI files. Hopefully the linter now succeeds as well. |
Looks great @threema-lenny ! Fwiw I changed the PR title over to "feat:" to trigger a release when this merges. Going to go ahead and merge this. |
# [1.152.0](v1.151.1...v1.152.0) (2023-07-10) ### Features * Ensure strict(er) TS compliance for the generated code ([#868](#868)) ([1405d4b](1405d4b))
🎉 This PR is included in version 1.152.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks a lot for the quick review. 👍 |
Changes:
tsc:check
script to ensure that there are no TS files with undetected errors.integrations
so the compiled proto TS files can underly much stricter configuration requirements than the test files themselves.DOM
tocompilerOptions.lib
for integration tests to have DOM'sAbortSignal
in scope.tsc:check
to the CI scriptThere were a bunch of type errors in the integration tests that weren't caught by anything. I honestly don't know Jest enough to understand why that happens but at least the new
tsc:check
script catches them correctly.I've applied the new
unwrap
utility in a couple of instances where I'm not precisely sure what the intended behaviour is, so this is a best effort guess. In any case, it doesn't make the experience any worse because those were uncaught error cases.Resolving unused parameters
noUnusedParameters: true
is probably a Sisyphean task but it would be great to resolve the very few areas where there were unused localsnoUnusedLocals: true
in the future.Note: Bumping Node types from 14 to 16 was kinda arbitrary. Node 16 is almost EOL, too.