perf: Improve performance by caching the isUsingTranspiler function#680
perf: Improve performance by caching the isUsingTranspiler function#680amilajack merged 2 commits intoamilajack:mainfrom
isUsingTranspiler function#680Conversation
Cache the isUsingTranspiler per directory
isUsingTranspiler functionisUsingTranspiler function
|
thanks for this, happy to merge after fixing merge conflicts |
|
Updated @amilajack |
There was a problem hiding this comment.
Pull request overview
This PR improves performance by memoizing the isUsingTranspiler function and fixing a bug where the function was incorrectly passing a filename instead of a directory to findUp. The changes include adding lodash.memoize to cache file system lookups by directory, and an additional optimization that caches getUnsupportedTargets results per rule within each context. Benchmarks show 6-36% performance improvements across various repositories.
Changes:
- Memoized
isUsingTranspilerfunction to cache babel config detection by directory - Fixed bug where filename was passed to
findUpinstead of directory - Added per-context cache for
getUnsupportedTargetsto avoid redundant computations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/rules/compat.ts | Refactored isUsingTranspiler to use memoization with file path parameter instead of context, fixed directory extraction bug, added unsupportedTargetsByRule cache for additional optimization |
| test/compat-isUsingTranspiler.spec.ts | Added comprehensive integration tests covering babel config detection, memoization behavior, file path handling, and package.json babel property detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return false; | ||
| }, | ||
| (filePath: string) => path.resolve(path.dirname(filePath)) |
There was a problem hiding this comment.
The memoization resolver should resolve the file path to an absolute path before extracting the directory. Currently, if a relative file path is passed, path.dirname(filePath) returns a relative directory, and path.resolve resolves it relative to process.cwd(). This could cause cache misses if the same directory is referenced with different relative path formats. Change to path.dirname(path.resolve(filePath)) to ensure consistent cache keys.
|
🎉 This PR is included in version 6.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What this PR is doing?
We are memoizing the
isUsingTranspiler. File reads are not great in large mono repositories. Where I work this is a bottleneck. Specifically for directories with a large number of files this can blow up the time a lot.This PR also fixes a minor bug. The function took the
contextand computed the directory likeconst dir = context.filename ?? context.getFilename();.The problem is that this is passing the file name instead of the expected directory to
findUp. This is also fixed in this PR. I have added tests as well.Benchmarks
Gist: https://gist.github.com/igeligel/af1961949a79d91e4855fc017b1d75ef