dividab / tsconfig-paths

Load node modules according to tsconfig paths, in run-time or via API.
MIT License
1.8k stars 100 forks source link

Skip stat call / throwing an exception when source files don't exist #225

Closed robstolarz closed 1 year ago

robstolarz commented 1 year ago

I noticed that as the number of tried file paths goes up, Node.js spends a lot of time preparing a native exception to throw over the JS barrier. I narrowed this down to be a result of relying on the fs.statSync call in fileExistsSync to determine whether or not the file exists on top of whether or not it is actually a file.

Prepending a call to fs.existsSync allows us to skip the preparation time of throwing an exception at the cost of causing another IO hit. There is ostensibly some sort of tradeoff but for the large monolithic app I work on, this shaves off the vast majority of the boot and hot reload time.[1]

Happy to discuss / prepare a more detailed breakdown if necessary.

1: 6x improvement in boot time on ~6k JS source files

maschwenk commented 1 year ago

Unbelievable find @robstolarz. Want to note to the maintainer and anyone who sees this PR, if you try to upgrade and use this, you may notice that you don't see the 6x benefit immediately. However, what I found is that if you for some reason have two import 'tsconfig-paths/register';'s in your code, perhaps because you are importing a package into an app that uses it, you could have a case where the improvements from this PR are being overridden when that second case of import 'tsconfig-paths/register' happens. It may make sense to notify folks who depend on this package:

  1. Of this incredible improvement
  2. Be careful that the monkey patch is not being undone. In truth, I think tsconfig-paths should log a warning when it detects that another instance of tsconfig-paths has already monkey patched require (if that's even possible. if it is lmk happy to upstream it)
robstolarz commented 1 year ago

@jonaskello Any thoughts on when you might be able to review this? We'd like to get this change moving as soon as possible

codecov[bot] commented 1 year ago

Codecov Report

Base: 68.28% // Head: 68.16% // Decreases project coverage by -0.11% :warning:

Coverage data is based on head (851596a) compared to base (206e49a). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #225 +/- ## ========================================== - Coverage 68.28% 68.16% -0.12% ========================================== Files 9 9 Lines 309 311 +2 Branches 95 96 +1 ========================================== + Hits 211 212 +1 - Misses 92 93 +1 Partials 6 6 ``` | [Impacted Files](https://codecov.io/gh/dividab/tsconfig-paths/pull/225?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dividab) | Coverage Δ | | |---|---|---| | [src/filesystem.ts](https://codecov.io/gh/dividab/tsconfig-paths/pull/225/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dividab#diff-c3JjL2ZpbGVzeXN0ZW0udHM=) | `72.00% <100.00%> (-1.92%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dividab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dividab)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jonaskello commented 1 year ago

Seems like a straightforward change so I think we can just merge and release in a patch version.

jonaskello commented 1 year ago

Release in 4.1.1

robstolarz commented 1 year ago

Thanks so much for the quick turnaround!!