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
threema-lenny
changed the title
|
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.
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. |
stephenh
changed the title
|
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:checkscript to ensure that there are no TS files with undetected errors.integrationsso the compiled proto TS files can underly much stricter configuration requirements than the test files themselves.DOMtocompilerOptions.libfor integration tests to have DOM'sAbortSignalin scope.tsc:checkto 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:checkscript catches them correctly.I've applied the new
unwraputility 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: trueis probably a Sisyphean task but it would be great to resolve the very few areas where there were unused localsnoUnusedLocals: truein the future.Note: Bumping Node types from 14 to 16 was kinda arbitrary. Node 16 is almost EOL, too.