-
-
Notifications
You must be signed in to change notification settings - Fork 52
fix: take context.cwd
into account for cache key
#388
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
Conversation
🦋 Changeset detectedLatest commit: 8311c98 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 |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
WalkthroughThis change refactors the caching mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant ESLint
participant ImportXPlugin
participant ModuleCache
participant FileSystem
ESLint->>ImportXPlugin: Lint file (with context)
ImportXPlugin->>ModuleCache: Generate cache key (includes cwd)
ModuleCache->>ModuleCache: Check cache for key
alt Cache hit and fresh
ModuleCache-->>ImportXPlugin: Return cached result
else Cache miss or stale
ImportXPlugin->>FileSystem: Resolve module/import
FileSystem-->>ImportXPlugin: Resolution result
ImportXPlugin->>ModuleCache: Set cache entry
end
ImportXPlugin-->>ESLint: Linting result
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
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
src/node-resolver.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/utils/module-cache.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/utils/resolve.spec.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (21)
✨ 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 (
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes cache key generation by incorporating the current working directory (process.cwd()) and improves the handling of cache settings and resolver instantiation. Key changes include:
- Updating type definitions to use NormalizedCacheSettings instead of the previous ImportSettings['cache'].
- Changing the cache key from an unknown type to a string and ensuring stale cache handling.
- Adjusting cache key generation in export-map.ts to include process.cwd(), and refactoring the node resolver to use a singleton instance.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/utils/resolve.ts | Updated cacheSettings parameter type to NormalizedCacheSettings. |
src/utils/module-cache.ts | Changed cache key type to string, and added cache clear on miss. |
src/utils/export-map.ts | Prepending process.cwd() to the context cache key for accuracy. |
src/types.ts | Added NormalizedCacheSettings interface to support new cache keys. |
src/node-resolver.ts | Refactored resolver instance handling; improved builtin check. |
.changeset/*.md | Updated changeset notes to reflect the fix and performance update. |
Comments suppressed due to low confidence (1)
src/utils/export-map.ts:1135
- [nitpick] Adding a comment to explain the inclusion of process.cwd() in the cache key could help future maintainers understand its impact on cache behavior.
process.cwd() +
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
+ Coverage 96.05% 96.09% +0.03%
==========================================
Files 96 96
Lines 4947 4944 -3
Branches 1855 1831 -24
==========================================
- Hits 4752 4751 -1
+ Misses 194 192 -2
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 73d1422 in 1 minute and 14 seconds. Click for details.
- Reviewed
273
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
draft 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. .changeset/huge-streets-fold.md:1
- Draft comment:
Changeset message is clear; no issues here. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. .changeset/major-apples-invent.md:1
- Draft comment:
Changeset message clearly states the fix for including process.cwd() in the cache key. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. src/node-resolver.ts:39
- Draft comment:
Using 'isBuiltin' from 'node:module' increases clarity over the previous 'module.isBuiltin' check. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. src/types.ts:159
- Draft comment:
The new NormalizedCacheSettings interface properly extends cache settings with a 'lifetime' field. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. src/utils/export-map.ts:1118
- Draft comment:
Child context now explicitly extracts 'cwd', 'filename', and 'physicalFilename' for consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. src/utils/export-map.ts:1135
- Draft comment:
Including process.cwd() at the beginning of the cache key addresses #370. Consider if frequent cwd changes could lead to cache thrashing. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. src/utils/module-cache.ts:29
- Draft comment:
Consider clearing expired cache entries from the map to prevent potential memory buildup in long-running processes. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. src/utils/resolve.ts:246
- Draft comment:
The cache key in fullResolve now combines the source directory, child context hash (which includes process.cwd()), and settings hash. Adding inline comments here could aid future maintainers. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_QB9vzbT7oBTobVDi
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed ac417f7 in 2 minutes and 15 seconds. Click for details.
- Reviewed
269
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
6
draft 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/node-resolver.ts:48
- Draft comment:
Consider logging or otherwise handling errors in the catch block rather than silently swallowing them; this will aid in debugging resolver failures. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a module resolver used by eslint-plugin-import. Failed resolutions are expected and common - they happen whenever a module isn't found. Logging errors would create noise in the logs. The function correctly handles the failure case by returning { found: false }. The empty catch block is actually the right approach here since this is part of the intended control flow. Maybe error logging could be useful for debugging genuine resolver issues vs expected "module not found" cases? The comment suggests it would aid debugging. In a resolver, failed resolution is a normal and expected outcome, not an error condition. Adding logging would create unnecessary noise that would make actual issues harder to debug. The comment should be deleted. The empty catch block is the correct implementation for this resolver pattern where failed resolution is an expected outcome.
2. src/types.ts:167
- Draft comment:
Add documentation (e.g. clear JsDoc comments) for the NormalizedCacheSettings interface and its 'lifetime' property to improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src/utils/export-map.ts:1135
- Draft comment:
Including process.cwd() in the cache key is effective for isolating caches across working directories; however, ensure that dynamic changes to process.cwd() don’t lead to unintended cache invalidation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is informative and suggests ensuring that dynamic changes toprocess.cwd()
don't lead to unintended cache invalidation. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't confirm if the behavior is intentional or not. Therefore, it violates the rules.
4. src/utils/module-cache.ts:36
- Draft comment:
Calling clearNodeResolverCache() within the get() method introduces a side-effect that clears the resolver cache for all modules; ensure that this behavior is intentional and document it accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is asking for documentation of behavior, but not pointing out an actual code issue. The side effect seems intentional as it's being added in this PR. The comment is essentially asking "are you sure?" which violates our rules. Documentation requests should be handled through other channels. The side effect could be important enough to warrant documentation. Clearing a global cache is a significant operation that affects other parts of the system. While the side effect is significant, the comment is still essentially asking for confirmation and documentation rather than pointing out a concrete issue that needs fixing. Per our rules, we should not ask authors to confirm their intentions. Delete the comment as it violates our rule about not asking authors to confirm their intentions or add documentation. The comment is not pointing out a concrete code issue that needs fixing.
5. src/utils/resolve.ts:247
- Draft comment:
The composed cache key now includes the working directory (via childContextHashKey) and a settings hash. Verify that changes to process.cwd() during runtime do not inadvertently invalidate valid cache entries. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/utils/resolve.ts:62
- Draft comment:
Using toLowerCase() to compare filepath and process.cwd() might not handle locale-specific or normalization issues; consider using a robust path normalization method for accuracy on case-insensitive file systems. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_4TiNIW42UfCnowQr
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed a4d7312 in 1 minute and 37 seconds. Click for details.
- Reviewed
273
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
5
draft 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/node-resolver.ts:29
- Draft comment:
Using a global singleton with cloneWithOptions is a neat performance optimization. Ensure that this state is acceptable and doesn’t lead to unintended side‐effects if options vary between calls. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the PR author to ensure that the state doesn't lead to unintended side-effects, which violates the rule against asking the author to ensure behavior is intended. However, it does provide a specific observation about the use of a global singleton with cloneWithOptions, which could be useful. The comment could be rephrased to focus on the potential issue without asking the author to ensure anything.
2. src/node-resolver.ts:44
- Draft comment:
The non-null assertion (resolver!) is used assuming it’s always initialized. Consider a safeguard or comment explaining why this is safe. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The non-null assertion is actually safe here because resolver is always initialized in createNodeResolver before the returned object's resolve method can be called. The code structure makes this fairly clear. The suggestion to add a comment doesn't really improve code quality since the initialization is obvious from reading the code. The comment does point out a potential code safety issue that future maintainers might question. TypeScript's type system can't prove the initialization is safe. While true, the initialization is clear from the code structure and adding a comment would be redundant given how close the initialization is to the usage. Delete the comment as it suggests adding unnecessary documentation for something that is clear from the code structure.
3. src/utils/export-map.ts:1134
- Draft comment:
In makeContextCacheKey, process.cwd() is prepended to the cache key. If the context provides its own cwd, consider using it instead for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Looking at the code: 1. The function takes a context parameter that includes cwd 2. Earlier in the file, cwd is destructured from context and used 3. Using context.cwd would be more consistent with how other context properties are used 4. However, this is a cache key - using process.cwd() may be intentional to ensure consistent caching across different context objects 5. Without understanding the caching requirements fully, changing this could introduce subtle bugs The comment makes a reasonable suggestion for consistency, but changing cache key generation could have unintended consequences. We don't have enough context about the caching requirements. While consistency is good, cache key generation needs to be stable and predictable. Without strong evidence that using context.cwd is safe for caching, we should be conservative. The comment should be removed. While it makes a reasonable suggestion, changing cache key generation is risky without fully understanding the caching requirements.
4. src/utils/module-cache.ts:40
- Draft comment:
Clearing the node resolver cache on every cache miss may be too aggressive. Please verify that this behavior is intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify their intention, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
5. src/utils/resolve.ts:246
- Draft comment:
The cache key now incorporates process.cwd() and settings hash. Confirm that this change properly invalidates the cache when the working directory changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_fxDJlrrPJ5IEb1Kf
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 102c661 in 51 seconds. Click for details.
- Reviewed
269
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
draft 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. .changeset/huge-streets-fold.md:1
- Draft comment:
The performance changeset is well documented and clearly states the use of a singleton resolver instance. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. .changeset/major-apples-invent.md:1
- Draft comment:
The changelog clearly documents the fix to include process.cwd() for the cache key, addressing issue #370. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. src/node-resolver.ts:12
- Draft comment:
The clearNodeResolverCache function using optional chaining is well implemented for resetting the resolver cache. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. src/node-resolver.ts:29
- Draft comment:
The singleton pattern for the resolver is applied nicely by cloning with new options or creating a new instance; the non-null assertion (resolver!) at line 44 appears safe given the flow. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. src/types.ts:167
- Draft comment:
Adding the NormalizedCacheSettings interface improves type safety by explicitly requiring a 'lifetime' property in the cache settings. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. src/utils/export-map.ts:1135
- Draft comment:
Including process.cwd() in the cache key generation is a key improvement that should prevent false cache hits in a monorepo environment where the working directory may change. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining why a change is beneficial. It does not suggest any code changes or ask for confirmation on specific intentions. According to the rules, purely informative comments should be removed.
7. src/utils/module-cache.ts:36
- Draft comment:
Calling clearNodeResolverCache on a cache miss ensures stale resolver entries are cleared. Confirm that this side effect on every miss is the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. src/utils/resolve.ts:237
- Draft comment:
The fullResolve function now builds a cache key incorporating the child context hash and a memoized hash of settings (which includes process.cwd() via makeContextCacheKey), thus addressing cache collisions in multi-package monorepos. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_wBtvySSPzuWeXimP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed f947d64 in 49 seconds. Click for details.
- Reviewed
17
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft 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/utils/export-map.ts:1132
- Draft comment:
Consider providing a fallback for 'cwd' (e.g. using cwd || process.cwd()) to ensure the cache key is valid even if cwd isn’t set in the context. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_fFguvKKlBOuvcy7J
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
process.cwd()
into account for cache keycontext.cwd
into account for cache key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 779aa36 in 56 seconds. Click for details.
- Reviewed
26
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft 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/utils/export-map.ts:1119
- Draft comment:
Using '\0' as an explicit separator in the cache key concatenation avoids accidental collisions between context and path parts. Ensure that none of the components ever contain the null character. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%
<= threshold50%
The comment is informative and suggests ensuring that none of the components contain the null character. It doesn't ask for a specific code change or test, nor does it suggest an alternative approach. It seems to be more of a cautionary note rather than a direct actionable comment.
2. src/utils/export-map.ts:1131
- Draft comment:
Replacing process.cwd() with the passed-in 'cwd' from context ensures the cache key reflects the actual working directory intended by the caller. Verify that all contexts provide a valid 'cwd' value. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to verify that all contexts provide a valid 'cwd' value, which is against the rules as it asks for verification. It does not provide a specific code suggestion or point out a specific issue with the code.
Workflow ID: wflow_JTHrAwfAe1a0smIn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed cd30d0b in 1 minute and 17 seconds. Click for details.
- Reviewed
64
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
draft 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. .changeset/huge-streets-fold.md:1
- Draft comment:
Removed changeset content indicating a singleton resolver. Ensure that the release notes and commit message accurately reflect that a fresh resolver instance is now created (to handle cwd changes) instead of using a global singleton. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src/node-resolver.ts:33
- Draft comment:
Since 'resolver' is locally defined and always set, the non-null assertion (!) in 'resolver!.sync(...)' is unnecessary. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src/utils/module-cache.ts:33
- Draft comment:
Removing the call to clearNodeResolverCache() on cache miss is appropriate now that resolver creation respects the current cwd, but verify that cache invalidation in cases of cwd change is properly handled elsewhere. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_62w4eYOpm45ZEpSp
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed f88d289 in 1 minute and 36 seconds. Click for details.
- Reviewed
101
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
3
draft 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/node-resolver.ts:15
- Draft comment:
Refactored to use a local ResolverFactory instance. Ensure that any context.cwd passed in restOptions is used to compute unique cache keys. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/utils/module-cache.ts:33
- Draft comment:
Removed clearNodeResolverCache call on cache miss. Verify that cache invalidation is handled elsewhere without relying on global state. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to verify that cache invalidation is handled elsewhere, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
3. test/utils/resolve.spec.ts:454
- Draft comment:
Explicitly casting 'cacheSettings' as NormalizedCacheSettings improves type safety. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_deYtcWwkiEJqIQ4L
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 8311c98 in 1 minute and 30 seconds. Click for details.
- Reviewed
111
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
4
draft 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. .changeset/major-apples-invent.md:5
- Draft comment:
Updated changeset message now reflects use of context.cwd. Ensure documentation and tests align with this change. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that documentation and tests align with the change, which violates the rule against asking the author to ensure things are tested or documented. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. src/node-resolver.ts:15
- Draft comment:
Refactored createNodeResolver to use a local resolver instance instead of a global mutable one. Confirm that this does not unintentionally bypass shared caching logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking for verification about caching behavior, which violates our rules about not asking authors to verify/confirm things. Additionally, the removal of global state in favor of local instances is generally a good practice. The old code's caching was likely an implementation detail rather than a required feature, since it was marked @internal. Maybe the caching was actually an important performance optimization that shouldn't be removed without careful consideration? The @internal annotation and the fact that the cache clearing function was removed suggests this was not a critical feature. If performance becomes an issue, it can be addressed later with metrics. Delete this comment as it asks for verification and makes speculative concerns about caching without clear evidence of an actual problem.
3. src/utils/module-cache.ts:30
- Draft comment:
Removed call to clearNodeResolverCache on cache miss. Verify that cache invalidation now properly factors in context.cwd so that stale resolver caches aren’t retained. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. test/utils/resolve.spec.ts:457
- Draft comment:
Added explicit type assertion for cache settings improves type safety. This helps ensure that cache keys include the appropriate context. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the benefits of a change without suggesting any action or asking for clarification. It doesn't align with the rules for useful comments in a pull request review.
Workflow ID: wflow_xoTwzUsPNhaPy1ec
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
close #370
related microsoft/vscode-eslint#1994
process.cwd()
/context.cwd
can be changed withprocess.cwd('new path')
Important
Fix cache key generation by including
context.cwd
to improve cache accuracy and reduce collisions.context.cwd
in cache key generation inexport-map.ts
andmodule-cache.ts
to improve cache accuracy and reduce collisions.module.isBuiltin
withisBuiltin
innode-resolver.ts
.CacheKey
type tostring
inmodule-cache.ts
.NormalizedCacheSettings
interface intypes.ts
for cache settings.resolve.spec.ts
to validate cache key changes and ensure correct module resolution behavior.This description was created by
for 8311c98. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores