JS: Update the extractor to use TypeScript 4.7 #9235
Conversation
kaeluka
changed the title
There was a problem hiding this comment.
I don't see a test/implementation of moduleSuffixes.
It can wait until after this PR has been merged, but we should consider it.
https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-rc/#resolution-customization-with-modulesuffixes
It probably requires some changes in NodeModuleResolutionImpl.qll around the tryExtensions predicate.
Otherwise LGTM, with two small comments from a quick look.
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
|
I've now added a test for the moduleSuffixes feature as well! |
You've tested that TypeScript can figure out the types, but not that our own module resolution can figure out the imports. |
|
Yes, that's what I did :-) i know there's still work left after merging 👍 |
erik-krogh
changed the title
|
I've bumped the version to the stable release that just happened. The tests will still fail until the internal PR has been merged (see backref above). |
| /////////// | ||
|
|
||
| const ErrorMap = Map<string, Error>; |
There was a problem hiding this comment.
What AST do we extract for this?
There was a problem hiding this comment.
The Map<string,Error> is an ExpressionWithTypeArguments from TypeScript.qll.
But we did not show that in the printAst output, so I fixed that.
|
|
||
| type FirstString<T> = | ||
| T extends [infer S extends string, ...unknown[]] |
There was a problem hiding this comment.
Do we extract the extends string part?
Usually we've tried to extract enough of the AST to prevent spurious unused variable/import alerts. Since the extends string part could instead contain a reference to an imported name, we'd want to extract it.
The unused variable alert isn't that important anymore, but I still think we should try to maintain it.
There was a problem hiding this comment.
Yes we extract it, and I've added a test that it works with our unused variable query.
There was a problem hiding this comment.
Thanks for addressing my questions @erik-krogh!
Everything LGTM, but could you please run an evaluation before merging?
Evaluation was uneventful. |
Erik here:
A plain update to TypeScript 4.7 without surprises.
There is not much special happening, but TypeScript now supports
.mtsand.ctsfiles, so we had to add those to the extractor.Tests have been added that we can handle all the features mentioned in the change-notes.
I can't test it locally, but the tests passed on the release candidate, so I highly expect them to pass on this stable release.
The tests will fail here until the internal PR has been merged.