Eliminiate custom TS System instance #680
Conversation
|
| Name | Link |
|---|---|
| 🔨 Latest commit | 7d64367 |
This comment was marked as outdated.
This comment was marked as outdated.
geigerzaehler
force-pushed
the
no-custom-ts-sys
branch
from
7d64367
to
3186924
Compare
There was a problem hiding this comment.
Thanks @geigerzaehler, this is great! Just what I was hoping for. Given that it also handles the external repos just fine I'm confident we can merge this.
Just a few nitpicks left.
| const resolvedFileName = resolveSync(sanitizedSpecifier, containingFile, [ | ||
| ...DEFAULT_EXTENSIONS, | ||
| ...customCompilerExtensions, | ||
| ]); |
There was a problem hiding this comment.
I think this can go back as extensions in the createCustomModuleResolver scope (slight perf cost to unnecessarily keep re-creating this array with a spread operator).
There was a problem hiding this comment.
Makes sense and fixed
| @@ -91,12 +91,29 @@ export function createCustomModuleResolver( | |||
|
|
|||
| return { | |||
| resolvedFileName, | |||
| extension: virtualFileExtensions.includes(ext) ? ts.Extension.Js : ext, | |||
| extension: getExtension(resolvedFileName), | |||
There was a problem hiding this comment.
Why are we using a function with customCompilerExtensions.some and customCompilerExtensions.includes? Maybe we can make it consistent/fast with a Set + Set.has?
There was a problem hiding this comment.
Good point. I removed the function and made it more consistent.
The custom instance was previously responsible for resolving mapped paths to files handled by an external loader (e.g. `.vue` files) or treated as-is (e.g. `.png`). Now, `tsconfig-paths` does the path mapping resolution. Note that the custom system resolver was realy only hit for mapped paths. For non-mapped paths `resolveSync()` would already resolve the file.
geigerzaehler
force-pushed
the
no-custom-ts-sys
branch
from
3186924
to
8fc0cf9
Compare
|
This is so good! Thanks, @geigerzaehler. |
|
🚀 This pull request is included in v5.20.0. See Release 5.20.0 for release notes. Using Knip in a commercial project? Please consider becoming a sponsor. |
Follow-up to #673
The custom instance was previously responsible for resolving mapped paths to files handled by an external loader (e.g.
.vuefiles) or treated as-is (e.g..png). Now,tsconfig-pathsdoes the path mapping resolution.Note that the custom system resolver was really only hit for mapped paths. For non-mapped paths
resolveSync()would already resolve the file.That’s my best guess explanation why we can make this change. Please let me know if I overlooked something.
It also looks like this does make resolution faster. I ran it multiple times against the
TanStack/queryrepo (because it has the most time spent inresolveModuleName()) and I’m seeing a 5-20% decrease in time spent inresolveModuleName(). (Take this with a grain of salt, though, since it’s only a rudimentary evaluation.)