Skip to content
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

Memoize module resolution #177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleg-codaio
Copy link

In a large codebase, tsconfig-paths starts to introduce performance issues as many modules need to be loaded. In #72 (comment), @BJChen990 suggested introducing a cache to avoid hitting the filesystem multiple times for the same path. For me, this resulted in a ~50% speedup.

@Rush
Copy link

Rush commented Sep 29, 2021

We also encountered a lot of issues with performance, especially on Windows. fs.statSync is being called excessively - multiple times for the same file. I also see that every node_module resolution triggers the extension looking in multiple locations in the absolute directory. This seems very wasteful. I added some console.logs

// application loads require('function-bind') which exists in node_modules
Loading function-bind
Exists /code/nexus/portal/function-bind
Exists /code/nexus/portal/function-bind.js
Exists /code/nexus/portal/function-bind.json
Exists /code/nexus/portal/function-bind.node
Exists /code/nexus/portal/function-bind.jsx
Exists /code/nexus/portal/function-bind.ts
Exists /code/nexus/portal/function-bind.tsx
Exists /code/nexus/portal/function-bind/index.js
Exists /code/nexus/portal/function-bind/index.json
Exists /code/nexus/portal/function-bind/index.node
Exists /code/nexus/portal/function-bind/index.jsx
Exists /code/nexus/portal/function-bind/index.ts
Exists /code/nexus/portal/function-bind/index.tsx

@oleg-codaio
Copy link
Author

A few other improvement ideas:

  • Set addMatchAll to be false by default, otherwise tsconfig-paths will try to resolve even files within node_modules, which should be unnecessary
  • By default the list of extensions to search is initialized as
    extensions: Array<string> = Object.keys(require.extensions),
    , where .ts / .tsx extensions are at the end. Instead, set that explicitly to ['.ts', '.tsx', '.js'] (though this may depend on the codebase)
  • Optimize
    pathsToTry.push({ type: "file", path: physicalPath });
    to only stat the file by direct name if the requested path actually has an extension (i.e., if (path.extname(physicalPath)) { ... })

@Rush
Copy link

Rush commented Sep 30, 2021

Wow, adding addMatchAll: false directly to the installed js files made all the difference. I no longer see so many useless operations to check for file presence. Now I wonder what would be the best way to add this option without breaking the workflow. Maybe an additional option group in tsconfig.json just like Angular does with its angularCompilerOptions ?

@jonaskello
Copy link
Member

jonaskello commented Sep 30, 2021

Have you checked the bootstrapping example in the readme? Calling the tsconfig-paths API in your own wrapper scripts gives you more control.

EDIT: You can see all the params of the register call in the readme.

@Rush
Copy link

Rush commented Sep 30, 2021

Honestly I like the simplicity of -r tsconfig-paths/register as this way I can also run various utility scripts in my repo without having a custom wrapper/entrypoint for each.

But I guess I could build my own simple -r ./setup-tsconfig-paths.js

@Rush
Copy link

Rush commented Sep 30, 2021

Btw. on Windows the default fileExists checking is absolutely insane. Insanely slow.
image
image
image

There are few reasons:

  1. Poor implementation by Node.JS
  2. Windows filesystem is slower than Linux
  3. aforementioned lack of caching and sub-optimal algorithms in tsconfig-paths

@jonaskello
Copy link
Member

If you want a quick solution for filesystem caching you could probably make a bootstrap scripts that calls the lower level creatematchpath. This function accepts a fileExists function that you can pass in. You could pass a function that cache in a way that suits your needs.

@Rush
Copy link

Rush commented Sep 30, 2021

@jonaskello I think it's better to figure out a solution for everyone. So far I'm unblocked by hacking the module directly, but let's figure out a proper code fix.

@jonaskello
Copy link
Member

Having a bootstrap script is probably a better way to figure out a solution than hacking the installed script. Once you have a bootstrap script that works well you could contribute back that solution either as an example snippet or a PR to the core API. As inspiration for using createMatchPath here is one example of using it to load esm modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants