Improve formatter performance with ESLint caching - #1214
Conversation
🦋 Changeset detectedLatest commit: 6b3a21a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds caching for ChangesESLint Caching and Conditional Logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
There was a problem hiding this comment.
Pull request overview
This PR improves prettier-eslint runtime performance by reusing ESLint setup work across repeated format/analyze calls and by skipping expensive log serialization unless verbose logging is enabled.
Changes:
- Add in-process caches for ESLint instances and resolved ESLint configs (keyed via
stable-hash-x). - Gate expensive
util.inspect-based log serialization behind debug/trace checks. - Update tests/mocks and add a patch changeset; bump a few dependencies.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils.ts |
Adds ESLint instance cache, exports shared logger + debug/trace helpers. |
src/index.ts |
Adds ESLint config cache and guards verbose log serialization. |
test/index.spec.ts |
Adds tests for caching behavior and trace logging; adjusts existing assertions. |
__mocks__/loglevel-colored-level-prefix.ts |
Enhances logger mock to support getLevel/levels behavior used by new helpers. |
package.json |
Adds stable-hash-x dependency; bumps test/dev deps. |
yarn.lock |
Lockfile updates for new/updated dependencies. |
.changeset/quiet-llamas-format.md |
Patch changeset describing the performance improvements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/index.ts (1)
361-364: ⚡ Quick winNormalize
filePathbefore hashing to improve cache reuse.Line 363 hashes raw
filePath. Equivalent references (relative vs absolute, different separators) can create separate keys for the same file and reduce cache effectiveness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 361 - 364, The filePath variable is being hashed directly without normalization, causing different representations of the same path (relative vs absolute paths, different separators) to generate different cache keys and reduce cache effectiveness. Normalize the filePath before passing it to the hash function call that creates the cacheKey. Apply path normalization to the filePath variable to ensure consistent path representations are used when computing the hash with the hash function.src/utils.ts (1)
687-701: ⚡ Quick winDeduplicate concurrent cache misses for the same key.
If two calls hit Lines 687-701 concurrently with equivalent options, both can instantiate
ESLintbefore either writes toeslintCache. Consider memoizing in-flight promises to fully realize the caching win under parallel formatting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils.ts` around lines 687 - 701, The current caching implementation in the ESLint initialization logic (around the eslintCache.get(cacheKey) and eslintCache.set calls) has a race condition where concurrent requests with identical eslintOptions can both miss the cache and instantiate separate ESLint instances. To fix this, introduce a separate in-flight promises cache alongside the eslintCache. When a cache miss occurs, check if a promise for that cacheKey is already in-flight; if so, return that promise to wait for the first instantiation to complete. If not in-flight, store the instantiation promise in the in-flight cache before importing and creating the new ESLint instance, then move the result to the regular eslintCache once complete and remove it from the in-flight cache.test/index.spec.ts (1)
484-484: 💤 Low valueInconsistent mock cleanup: mockReset vs mockClear.
Line 484 uses
eslintMock.ESLint.mockReset()while line 292 (inbeforeEach) usesmockClear(). Both clear call history, butmockReset()also removes any mock implementation and return values.Using
mockReset()here is likely unnecessary and inconsistent with the general cleanup strategy. If the test needs to fully reset the mock for a specific reason, add a comment explaining why.♻️ Align with mockClear convention
).rejects.toThrow(error); - eslintMock.ESLint.mockReset(); + eslintMock.ESLint.mockClear(); expect(logger.error).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/index.spec.ts` at line 484, The mock cleanup on line 484 for eslintMock.ESLint uses mockReset() while the beforeEach hook on line 292 uses mockClear(). Since mockReset() is more aggressive (clearing both call history and implementation), align the cleanup strategy by changing mockReset() to mockClear() on line 484 to match the convention used elsewhere. If mockReset() is intentionally needed for a specific reason in this context, add a comment explaining why the stronger reset is necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Line 29: The eslintConfigCache Map has no size limit and will grow
indefinitely as new entries are added during cache insertion operations in the
lines where entries are set to the cache. To fix this, implement a bounded cache
mechanism by adding a maximum size limit to eslintConfigCache (such as 100 or
200 entries depending on typical usage patterns) and add eviction logic that
removes the oldest or least recently used entry when the cache reaches its size
limit before inserting new entries. This prevents unbounded memory growth in
long-running processes while maintaining cache functionality.
In `@src/utils.ts`:
- Around line 33-34: The eslintCache Map at line 33 has no bounds and will grow
unbounded as new ESLint configurations are inserted at lines 687-701, causing
memory to accumulate in long-lived sessions. Replace the unbounded Map with a
bounded cache implementation that enforces a maximum size limit (such as LRU or
similar eviction strategy). When inserting new entries into the cache at lines
687-701, ensure the cache respects its size limit by evicting entries when
capacity is exceeded. This prevents the eslintCache from retaining heavy ESLint
instances indefinitely.
In `@test/index.spec.ts`:
- Line 292: The test isolation is broken because module-level caches
(eslintCache in src/utils.ts and eslintConfigCache in src/index.ts) are not
being cleared between tests. In src/utils.ts, export a clearCaches function that
clears the eslintCache Map. In src/index.ts, export a clearESLintConfigCache
function that clears the eslintConfigCache Map. Then in the test file's
beforeEach block (which currently only clears the eslintMock), import and call
both of these cache-clearing functions to ensure caches are reset before each
test runs.
- Around line 423-424: The test "resolves to the local eslint module" currently
only verifies that prettierMock.format and eslintMock.mock.lintText were called,
but does not actually validate that the correct ESLint module was resolved. Add
an assertion after the existing expect statements to verify that
globalThis.__PRETTIER_ESLINT_TEST_STATE__.eslintPath is defined, which will
confirm the proper ESLint module path was resolved during the test execution and
catch regressions where the wrong module is loaded.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 361-364: The filePath variable is being hashed directly without
normalization, causing different representations of the same path (relative vs
absolute paths, different separators) to generate different cache keys and
reduce cache effectiveness. Normalize the filePath before passing it to the hash
function call that creates the cacheKey. Apply path normalization to the
filePath variable to ensure consistent path representations are used when
computing the hash with the hash function.
In `@src/utils.ts`:
- Around line 687-701: The current caching implementation in the ESLint
initialization logic (around the eslintCache.get(cacheKey) and eslintCache.set
calls) has a race condition where concurrent requests with identical
eslintOptions can both miss the cache and instantiate separate ESLint instances.
To fix this, introduce a separate in-flight promises cache alongside the
eslintCache. When a cache miss occurs, check if a promise for that cacheKey is
already in-flight; if so, return that promise to wait for the first
instantiation to complete. If not in-flight, store the instantiation promise in
the in-flight cache before importing and creating the new ESLint instance, then
move the result to the regular eslintCache once complete and remove it from the
in-flight cache.
In `@test/index.spec.ts`:
- Line 484: The mock cleanup on line 484 for eslintMock.ESLint uses mockReset()
while the beforeEach hook on line 292 uses mockClear(). Since mockReset() is
more aggressive (clearing both call history and implementation), align the
cleanup strategy by changing mockReset() to mockClear() on line 484 to match the
convention used elsewhere. If mockReset() is intentionally needed for a specific
reason in this context, add a comment explaining why the stronger reset is
necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fdfbacf-31f5-41fc-b251-2235a7a8d7e2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.changeset/quiet-llamas-format.md__mocks__/loglevel-colored-level-prefix.tspackage.jsonsrc/index.tssrc/utils.tstest/index.spec.ts
a689079 to
8109eed
Compare
8109eed to
6b3a21a
Compare
Summary
stable-hash-xto avoid repeated setup work.pretty-formatdirectly only when verbose logging is enabled.Test plan
yarn vitest run --coverageyarn eslint . --cacheyarn buildyarn formatSummary by CodeRabbit