fix: include options hash in cache key for options normalization #466
Conversation
🦋 Changeset detectedLatest commit: 769f6a6 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 |
|
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. |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 614f3b2 in 38 seconds. Click for details.
- Reviewed
146lines of code in7files - Skipped
1files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/normalize-options.ts:19
- Draft comment:
Good change: updating configFileMapping to Map<string, string> ensures only the resolved filename is cached, avoiding reuse of stale options. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/normalize-options.ts:79
- Draft comment:
The caching block now correctly checks for a cached resolved config file and re-resolves using tryFile when needed. This addresses issue Caching results in wrong configuration being used #465 by decoupling caching from provided options. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_FAEdDSxudN6mmQHT
You can customize 
commit: |
Presumably to avoid excessive stat calls, the return value of `normalizeOptions()` is cached for each input tsconfig file. But the return value also depends on the input `options`: eslint configuration may pass different options for the same tsconfig file. Instead, cache only the result of the `tryFile()` call.
614f3b2 to
aba5ddf
Compare
|
""" WalkthroughThe changes enhance the caching mechanism in the TypeScript resolver by including a stable hash of the entire options object in the cache key, ensuring distinct caching for different option sets. New end-to-end tests and supporting files verify correct resolution behavior for files with differing resolver options and extensions. Changes
Sequence Diagram(s)sequenceDiagram
participant ESLint
participant Resolver
participant Cache
ESLint->>Resolver: Resolve module with options (per file)
Resolver->>Cache: Lookup cache with key: configFile + optionsHash
alt Cache hit
Cache-->>Resolver: Return cached normalized options
else Cache miss
Resolver->>Resolver: Normalize options
Resolver->>Cache: Store normalized options with key: configFile + optionsHash
end
Resolver-->>ESLint: Return resolution result
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Ok, we can do it that way. 🤷 I didn't move the check though, because the separate cache in |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with caching options in normalizeOptions by incorporating an options hash into the cache key. It also adds tests to ensure that different option sets for the same tsconfig file are correctly handled.
- Updates the caching mechanism in src/normalize-options.ts by computing a stable hash for the options.
- Introduces new test files and adjustments in the ESLint configuration to validate the behavior with different file extensions.
- Adds a minimal tsconfig.json and enhanced snapshots for the new test scenarios.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/filesWithDifferentOptions/tsconfig.json | Added minimal config file for tests |
| tests/e2e/filesWithDifferentOptions/src/y.bar.js | Added test file exporting a constant |
| tests/e2e/filesWithDifferentOptions/src/x.foo.js | Added test file exporting a constant |
| tests/e2e/filesWithDifferentOptions/src/a.foo.js | Uses default import for x despite x.foo.js exporting a named export |
| tests/e2e/filesWithDifferentOptions/src/a.bar.js | Uses default import for y despite y.bar.js exporting a named export |
| tests/e2e/filesWithDifferentOptions/eslint.config.js | Configured extension preferences for .foo.js and .bar.js files |
| tests/e2e/snapshots/e2e.spec.ts.snap | Updated snapshot for new test scenarios |
| src/normalize-options.ts | Updated caching implementation to include options hash in the cache key |
| .changeset/slick-breads-feel.md | Documented the patch update |
Comments suppressed due to low confidence (2)
tests/e2e/filesWithDifferentOptions/src/a.foo.js:1
- The file is importing the default export from './x', but x.foo.js exports x as a named export. Consider changing the import to use named import syntax (e.g., import { x } from './x') or updating x.foo.js to export a default.
import x from './x'
tests/e2e/filesWithDifferentOptions/src/a.bar.js:1
- The file is importing the default export from './y', but y.bar.js exports y as a named export. Consider changing the import to use named import syntax (e.g., import { y } from './y') or updating y.bar.js to export a default.
import y from './y'
Presumably to avoid excessive stat calls, the return value of
normalizeOptions()is cached for each input tsconfig file. But the return value also depends on the inputoptions: eslint configuration may pass different options for the same tsconfig file.To fix it, include a hash of the
optionsin the cache key too.Fixes #465
Important
Modify
normalizeOptions()to cache only filename lookup, ensuring correct handling of different options for the same tsconfig file, and add tests for verification.normalizeOptions()innormalize-options.tsto cache only the filename lookup, not the entire options object.filesWithDifferentOptionstest case ine2e.spec.ts.snapto verify behavior with different file extensions.eslint.config.js,a.bar.js,a.foo.js,x.foo.js,y.bar.jsadded undertests/e2e/filesWithDifferentOptions/to support the new test case.This description was created by
for 614f3b2. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
.foo.jsand.bar.jsextensions.Chores