feat: implement a new options argument for resolver with 100% compatibility
#335
Conversation
🦋 Changeset detectedLatest commit: ab2d06f 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 update introduces an Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Resolver
participant LegacyResolver
participant ExportMap
participant Utils
Caller->>Resolver: resolve(source, file, options)
alt New-style resolver
Resolver->>Utils: Use options.context and options.tsconfig
Utils->>ExportMap: parse(..., context)
ExportMap->>Utils: getTsconfigWithContext(context)
else Legacy resolver
Resolver->>LegacyResolver: resolve(source, file, config, _, options)
LegacyResolver->>Utils: Use options.context and options.tsconfig
end
Resolver-->>Caller: { found, path }
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/utils/export-map.tsOops! Something went wrong! :( ESLint: 9.27.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/fixtures/foo-bar-resolver-no-version.jsOops! Something went wrong! :( ESLint: 9.27.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/utils/resolve.tsOops! Something went wrong! :( ESLint: 9.27.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 (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code Graph Analysis (1)src/utils/export-map.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms (180)
🔇 Additional comments (6)
✨ 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 (
|
WalkthroughThis pull request introduces a new Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
|
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: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 2788d9e in 2 minutes and 24 seconds. Click for details.
- Reviewed
505lines of code in11files - Skipped
0files when reviewing. - Skipped posting
12draft 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. resolvers/README.md:11
- Draft comment:
Documentation now covers the new 'options' argument. Consider adding an explicit example showing how to use options.context and options.tsconfig to improve clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/types.ts:147
- Draft comment:
The ChildContext interface now includes a mandatory 'cwd' field. Ensure that this new requirement is documented for consumers of the API. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/types.ts:61
- Draft comment:
The new ResolveOptions interface correctly combines extra tsconfig info with context. Verify that README examples remain consistent with these types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/utils/export-map.ts:163
- Draft comment:
Passing an empty options object to the resolve() call ensures compatibility with the new resolver signature. This change looks correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/utils/export-map.ts:440
- Draft comment:
The function 'getTsconfigInternal' now returns the tsconfig object (rather than a boolean) and its value is later used to compute esModuleInterop. Consider renaming or adding a comment to clarify its role for future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/utils/resolve.ts:290
- Draft comment:
The legacy compatibility wrapper in 'importXResolverCompat' correctly adapts legacy resolvers by passing the new options object (with a lazy 'tsconfig' getter). This approach maintains backward compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. resolvers/README.md:15
- Draft comment:
The anchor foroptions.tsconfigis written as#optionstsconfig. Consider adding a hyphen (e.g.,#options-tsconfig) if that was the intended format. - 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 technically correct - the anchor link doesn't match the section heading format. However, this is a very minor markdown formatting issue. The link likely works fine either way since GitHub's anchor link generation is quite flexible. This feels like an overly pedantic comment that doesn't meaningfully improve the code. The comment is technically accurate and could improve consistency. Bad anchor links could cause navigation issues. While technically correct, this is an extremely minor markdown formatting issue that doesn't affect functionality. The documentation is still clear and usable. This comment should be removed as it's too minor and doesn't meaningfully improve the code or documentation.
8. resolvers/README.md:228
- Draft comment:
Typo: Consider changing "For compatibility reason" to "For compatibility reasons" to correct the grammatical error. - 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% While the comment is technically correct about the grammar, it's an extremely minor issue that doesn't affect understanding. The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category - it's a trivial documentation fix that doesn't impact functionality. The grammar error could make the documentation look less professional. Documentation quality is important for open source projects. While documentation quality matters, this is such a minor issue that it creates more noise than value in the PR review process. The meaning is completely clear despite the small grammatical error. Delete this comment as it's too minor and doesn't require immediate action. It violates the rule about not making purely informative or unimportant comments.
9. resolvers/README.md:245
- Draft comment:
Typographical error: The function uses 'opts(file, config)' but the function signature now includes an 'options' parameter. Should 'opts' be changed to 'options' to avoid an undefined variable? - 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 appears to be incorrect. Theoptsfunction call is likely a helper function defined elsewhere that takes file and config parameters to generate options, not related to theoptionsparameter. Theoptionsparameter is the 5th parameter and is used differently. This is a v2 example showing backward compatibility. I could be wrong about theoptsfunction - maybe it is a typo and should use the options parameter. But without seeing the full implementation, I can't be sure. Given this is documentation of a real implementation and has been working, it's more likely thatoptsis an intentional helper function rather than a typo. The comment is making assumptions without full context. The comment should be deleted as it appears to be incorrect -optsis likely a separate helper function and not related to theoptionsparameter.
10. test/fixtures/foo-bar-resolver-no-version.js:13
- Draft comment:
Typographical suggestion: In the error message, consider changing "thecontext.cwdmust be presented" to "thecontext.cwdmust be present" for clarity. - 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 suggestion is technically correct - "must be present" is more grammatically correct than "must be presented". However, the meaning is still clear either way. This is a very minor change that doesn't affect functionality. The comment is about changed code, but it's an extremely minor stylistic issue. The error message could be confusing to non-native English speakers in its current form. Error messages are part of the API and should be clear. While clarity is important, the current wording still effectively communicates the error condition. This is a test file, not public API. The comment should be deleted as it's too minor of an issue to warrant a PR comment. It violates the rule about not making obvious or unimportant comments.
11. test/fixtures/foo-bar-resolver-v2.js:13
- Draft comment:
Typo: Consider changing "thecontext.cwdmust be presented" to "thecontext.cwdmust be present" for proper phrasing. - 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% While the comment is technically correct (the word "present" is more grammatically correct than "presented" here), this is a very minor issue in a test assertion message. Test assertion messages are primarily for debugging and don't need to be perfectly grammatical as long as they're clear. The meaning is still perfectly understandable. The grammatical error could potentially confuse non-native English speakers. Also, maintaining high quality even in test code can be important for project standards. While good grammar is nice, this is an extremely minor issue in test code that doesn't impact functionality or clarity. The current message is still completely understandable. Delete the comment. The grammatical suggestion, while technically correct, is too minor to warrant a PR comment, especially for test code.
12. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Typo noticed in the assertion message: "thecontext.cwdmust be presented". Consider using "present" instead of "presented" to accurately indicate that the property should exist. - 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% While the comment is technically correct (the word "presented" is grammatically incorrect here), this is a very minor issue in a test fixture file. The meaning is still clear despite the grammatical error. The comment doesn't point out any functional issues or code quality problems. Test assertion messages are primarily for developer consumption and don't affect functionality. The grammatical error could make the error message slightly confusing to non-native English speakers. Clear error messages are important for debugging. While clear error messages are good, this particular grammatical error is minor and the meaning remains obvious. The benefit of fixing it doesn't justify the overhead of a PR comment. Delete this comment as it points out an extremely minor grammatical issue that doesn't impact code functionality or significantly affect readability.
Workflow ID: wflow_pjLDBzR6SlUA5ot6
You can customize 
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new options parameter to all resolver interfaces while preserving full backward compatibility.
- Updated tests and fixtures to pass and assert the new
options.context.cwd - Extended
fullResolve,relative,resolve, and legacy helper to acceptcontextandextraoptions - Added
ResolveOptionsExtraandResolveOptionstypes and updated README
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils.ts | Added cwd to test context |
| test/node-resolver.spec.ts | Updated resolver calls to pass { context } |
| test/fixtures/*.js | Augmented fixtures to accept _ and options arguments and assert options.context.cwd |
| src/utils/resolve.ts | Extended resolve functions (fullResolve, relative, resolve) to propagate context and extra |
| src/utils/legacy-resolver-settings.ts | Updated legacy resolver call signatures to include _ and options |
| src/utils/export-map.ts | Changed internal resolve calls to pass context/extra, added cwd to child contexts |
| src/types.ts | Introduced ResolveOptionsExtra, ResolveOptions, updated resolver signatures |
| resolvers/README.md | Documented new options parameter and its properties |
Comments suppressed due to low confidence (2)
test/node-resolver.spec.ts:31
- There are assertions for
options.context.cwd, but no tests verify thatoptions.tsconfigis forwarded. Consider adding a test to ensure thetsconfigproperty is passed throughoptions.
const result = resolver.resolve(source, _filename, {
test/node-resolver.spec.ts:5
- The import uses './utils.js' but the source file is 'utils.ts'. Please ensure the file extension matches the actual module (or configure your module resolver to handle .ts imports).
import { testContext } from './utils.js'
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #335 +/- ##
==========================================
+ Coverage 96.07% 96.27% +0.19%
==========================================
Files 93 93
Lines 4816 4853 +37
Branches 1821 1841 +20
==========================================
+ Hits 4627 4672 +45
+ Misses 188 180 -8
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/utils/export-map.ts (1)
1150-1160:⚠️ Potential issueAccessing
context.cwdmay throw – provide a fallback
RuleContextinstances have nocwdfield. Referencing it here breaks both type-checking and runtime in common ESLint environments.- cwd: context.cwd, + cwd: + 'cwd' in context && typeof (context as any).cwd === 'string' + ? (context as any).cwd + : process.cwd(),This aligns with the contract change proposed in
src/types.tsand prevents crashes whenchildContext()is called with a rawRuleContext.
♻️ Duplicate comments (1)
resolvers/README.md (1)
26-27: Same as above – convert bold lines to sub-headings to silence lint warnings and improve accessibility.
🧹 Nitpick comments (9)
test/fixtures/foo-bar-resolver-v3.js (1)
18-18: Added assertion to validate context.cwdThis assertion correctly validates that the
options.context.cwdproperty is present, ensuring the resolver receives the proper context information. This is important for maintaining the contract between callers and the resolver implementation.However, consider adding more specific error messages to help with debugging:
-assert.ok(options.context.cwd, 'the `context.cwd` must be presented') +assert.ok(options.context.cwd, 'options.context.cwd must be provided for resolver to function correctly')test/fixtures/foo-bar-resolver-v2.js (2)
4-4: Use_unusedor omit the placeholder for readabilityThe anonymous
_parameter is only there for back-compat. Renaming it to_unused(or documenting why it exists) makes the intent explicit and prevents future devs from wondering whether something is missing.
12-13: Prefer strict equality in assertions and fix grammar
assert.ok(!_)works but is less explicit thanassert.strictEqual(_, undefined).
Likewise, “must be presented” should read “must be present”.- assert.ok(!_, 'the 4th argument must be undefined') - assert.ok(options.context.cwd, 'the `context.cwd` must be presented') + assert.strictEqual(_, undefined, 'the 4th argument must be undefined') + assert.ok(options.context.cwd, 'the `context.cwd` must be present')test/fixtures/foo-bar-resolver-v1.js (1)
12-13: Keep the assertion message consistent with what you testThe code checks
options.context.cwd, yet the message only mentionscontext.- assert.ok(options.context.cwd, 'the `context` must be presented') + assert.ok(options.context.cwd, 'the `context.cwd` must be present')test/fixtures/foo-bar-resolver-no-version.js (1)
12-13: Minor wording & equality improvementsSame remark as for the v1/v2 fixtures – favour
strictEqualand correct the wording.- assert.ok(!_, 'the 4th argument must be undefined') - assert.ok(options.context.cwd, 'the `context.cwd` must be presented') + assert.strictEqual(_, undefined, 'the 4th argument must be undefined') + assert.ok(options.context.cwd, 'the `context.cwd` must be present')src/utils/legacy-resolver-settings.ts (2)
90-92: Type-annotateoptionsand makecontextnon-overridableUsing an explicit type improves IntelliSense and guarantees we always pass a full
ResolveOptions.
Spreadingextrafirst prevents any future accidental override ofcontext.- const options = { context, ...extra } + const options: ResolveOptions = { ...extra, context }
94-104: Bubble up resolver errors to aid debuggingThe catch-all swallows every error coming from legacy resolvers, making real configuration problems hard to diagnose.
Consider re-throwing the error (or at least logging it under a debug flag) instead of silently returning{ found: false }.resolvers/README.md (1)
14-16: Use proper Markdown headings to satisfyremark-lint/no-emphasis-as-headingThe bold text rendered as a pseudo-heading is triggering CI warnings.
Replace the strong tag with a real heading:- - **`options.context`** - - **`options.tsconfig`** + - #### `options.context` + - #### `options.tsconfig`This eliminates the warning and keeps the document structure clear.
src/utils/resolve.ts (1)
291-302: Guard againstoptions.tsconfigbeing lazily evaluated multiple timesInside the compatibility shim you re-expose
options.tsconfigvia a getter that calls the original getter each access.
Consider memoising to avoid repeatedgetTsconfig()IO if a legacy resolver calls it frequently:- { - get tsconfig() { - return options.tsconfig - }, - }, + (() => { + let cached = undefined as typeof options.tsconfig + return { + get tsconfig() { + return (cached ??= options.tsconfig) + }, + } + })(),A small optimisation but free insurance against excessive disk reads in resolver loops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
.changeset/four-pumas-leave.md(1 hunks)resolvers/README.md(8 hunks)src/types.ts(3 hunks)src/utils/export-map.ts(6 hunks)src/utils/legacy-resolver-settings.ts(3 hunks)src/utils/resolve.ts(7 hunks)test/fixtures/foo-bar-resolver-no-version.js(1 hunks)test/fixtures/foo-bar-resolver-v1.js(1 hunks)test/fixtures/foo-bar-resolver-v2.js(1 hunks)test/fixtures/foo-bar-resolver-v3.js(1 hunks)test/node-resolver.spec.ts(2 hunks)test/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/node-resolver.spec.ts (1)
test/utils.ts (1)
testContext(155-161)
src/utils/legacy-resolver-settings.ts (1)
src/types.ts (4)
ResolveOptions(61-63)ChildContext(147-156)RuleContext(140-145)ResolveOptionsExtra(57-59)
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
resolvers/README.md
[warning] 107-107:
{"reason":"Unexpected strong introducing a section, expected a heading instead","source":"remark-lint","ruleId":"no-emphasis-as-heading","severity":1}
[warning] 101-101:
{"reason":"Unexpected strong introducing a section, expected a heading instead","source":"remark-lint","ruleId":"no-emphasis-as-heading","severity":1}
⏰ Context from checks skipped due to timeout of 90000ms (190)
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
🔇 Additional comments (6)
.changeset/four-pumas-leave.md (1)
1-5: LGTM - Appropriate changeset description for featureThe changeset correctly describes this as a minor version update with a new feature that maintains compatibility. This aligns with semantic versioning principles since it's adding functionality in a backward-compatible manner.
test/utils.ts (1)
157-157: Addingcwdproperty to testContext is appropriateThis change adds the current working directory (
cwd) to the test context, which is necessary to support the newoptions.contextparameter in resolver interfaces. This ensures test utilities properly exercise the new functionality.test/node-resolver.spec.ts (2)
5-5: Import statement added correctlyThe import for
testContextfrom the utils file is properly added to support the new options parameter in resolver calls.
31-33: Resolver call updated properly with options argumentThe resolver.resolve call has been updated to include the new
optionsparameter with context information. This change properly tests the new API while maintaining the same functionality.test/fixtures/foo-bar-resolver-v3.js (2)
3-3: Added assertion dependencyThe
assert/strictmodule is correctly imported to support the new assertion for validating the options parameter.
10-10: Updated resolver interface to include options parameterThe resolve method signature has been appropriately updated to include the new
optionsparameter, maintaining the v3 interface version while adding the new capability.
JounQin
force-pushed
the
feat/resolver_options
branch
from
f9981d6 to
5e4a812
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 5e4a812 in 1 minute and 51 seconds. Click for details.
- Reviewed
588lines of code in12files - Skipped
0files when reviewing. - Skipped posting
17draft 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/four-pumas-leave.md:5
- Draft comment:
Release note is clear and concise. Verify consistency with other changelog entries. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. resolvers/README.md:12
- Draft comment:
Documentation update correctly reflects the new resolver signature with the third argument 'options'. Ensure anchor links (e.g., options.context) work as intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/types.ts:57
- Draft comment:
New types for resolver options (including the 'context' field) are well defined. Consider adding inline comments with usage examples for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/utils/export-map.ts:198
- Draft comment:
The integration of lazy tsconfig using a getter in the options parameter is neat. Ensure that caching via tsconfigCache handles null/undefined correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/utils/legacy-resolver-settings.ts:28
- Draft comment:
Legacy resolver function signatures now include the new options parameter while still passing undefined as the 4th argument. This preserves backward compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/utils/resolve.ts:171
- Draft comment:
fullResolve now accepts context and extra parameters and correctly passes them to both new and legacy resolvers. Review the usage in the compatibility wrapper. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. test/fixtures/foo-bar-resolver-no-version.js:12
- Draft comment:
Test assertions ensure the legacy resolver receives 'undefined' as fourth argument and that options.context.cwd is defined. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. test/fixtures/foo-bar-resolver-v1.js:12
- Draft comment:
Test for v1 resolver correctly verifies the new argument contract and interfaceVersion is set to 1. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. test/fixtures/foo-bar-resolver-v2.js:12
- Draft comment:
v2 resolver tests now check for the undefined fourth argument and validate presence of context. Good backward compatibility check. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
v3 resolver uses the new signature correctly by accepting the options parameter; assertions verify required context is present. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. test/node-resolver.spec.ts:31
- Draft comment:
Test now passes a context object (via testContext) into resolver.resolve. Snapshot tests validate the behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. test/utils.ts:155
- Draft comment:
Updated testContext now includes the 'cwd' property to support new resolver option requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. resolvers/README.md:230
- Draft comment:
Typo: The phrase "For compatibility reason" should be revised to "For compatibility reasons" for grammatical correctness. - 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% While the comment is technically correct about the grammar, it's an extremely minor issue that doesn't affect understanding. The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category - it's a trivial documentation fix that doesn't impact functionality. The grammar error could make the documentation look less professional. Documentation quality is important for open source projects. While documentation quality matters, this is such a minor grammatical issue that it doesn't significantly impact readability or professionalism. The meaning is completely clear either way. Delete this comment as it's too minor and doesn't meet the bar for PR review comments. It violates the rule about not making purely informative or unimportant comments.
14. resolvers/README.md:230
- Draft comment:
Typo: The phrase "support bothv2andv3interface" should likely be "support bothv2andv3interfaces" to accurately reflect plurality. - 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% While technically correct, this is an extremely minor grammatical issue that doesn't affect understanding. Documentation comments should focus on technical accuracy and clarity rather than perfect grammar. The meaning is perfectly clear either way. The grammar correction is technically accurate. Poor grammar in documentation could be seen as unprofessional. The grammatical issue is so minor that raising it as a PR comment creates more noise than value. The documentation's meaning is clear regardless. Delete this comment as it's too minor and doesn't meaningfully improve the documentation.
15. resolvers/README.md:247
- Draft comment:
Typo: The function signature now includes an 'options' parameter, but on this line it still callsopts(file, config). Please verify ifoptsshould be replaced withoptions. - Reason this comment was not posted:
Comment was on unchanged code.
16. test/fixtures/foo-bar-resolver-no-version.js:13
- Draft comment:
Typo notice: The error message reads "thecontext.cwdmust be presented". Consider revising it to "thecontext.cwdmust be provided" or "thecontext.cwdmust be present" for clarity. - 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% While the comment is technically correct (the word "presented" is slightly awkward English), this is a test fixture file, not production code. The error message is still perfectly understandable despite the minor grammatical issue. The benefit of fixing this is extremely minimal, especially in test code. We should focus on more substantial issues. The error message could potentially be seen by developers during test failures, so clarity in error messages has some value. Also, maintaining good English throughout the codebase, even in tests, sets a good standard. While clear error messages are valuable, this particular change is too minor to warrant a comment. The existing message is still clear enough to serve its purpose in a test file. Delete this comment as it suggests a trivial grammatical change in a test fixture file that doesn't meaningfully impact code quality or functionality.
17. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Typo: The error message says 'thecontext.cwdmust be presented'. Consider changing it to 'thecontext.cwdmust be present' (or 'provided') for improved clarity. - 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 about a real change in the diff - a new assertion statement. The grammatical error does make the error message slightly less clear. However, the meaning is still understandable, and this is just test code. The change would be minor and mostly cosmetic. Our rules say not to make purely informative comments or unimportant changes. The error message could confuse non-native English speakers. Clear error messages are important for debugging, even in test code. While clear error messages are good, this grammatical issue is minor and doesn't significantly impact understanding. The message's intent remains clear enough. Delete the comment as it suggests an unimportant change that doesn't materially impact the code's functionality or clarity.
Workflow ID: wflow_eIwhvslgKUCKFBtU
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
5e4a812 to
bc5ac1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/utils/export-map.ts (2)
291-295: Fix the tsconfig getter to never return null.The
ResolveOptionsExtra.tsconfigis typed asTsConfigJsonResolved | undefined, but the getter may yieldnullfromgetTsconfigInternal(), violating the declared type.get tsconfig() { - return tsconfig() // may be null + return tsconfig() ?? undefined }This prevents downstream strict-null errors by ensuring the type matches the interface definition.
199-201: Consider renaming local variable to avoid shadowing imported module.The local variable
pathshadows the importedpathmodule. This could cause confusion.- const path = resolve(source, context, { + const resolvedPath = resolve(source, context, { get tsconfig() { return tsconfig() }, }) - if (path == null) { + if (resolvedPath == null) { return null } - return ExportMap.for(childContext(path, context)) + return ExportMap.for(childContext(resolvedPath, context))
🧹 Nitpick comments (1)
src/utils/export-map.ts (1)
93-126: NewgetTsconfigInternalfunction looks good but consider adding error handling.The function effectively centralizes tsconfig retrieval and caching logic, but it doesn't handle potential exceptions from
getTsconfig. Consider adding try/catch to prevent crashes during config loading.function getTsconfigInternal(context: ChildContext | RuleContext) { const parserOptions = context.parserOptions || {} let tsconfigRootDir = parserOptions.tsconfigRootDir const project = parserOptions.project const cacheKey = stableHash({ tsconfigRootDir, project }) let tsConfig: TsConfigJsonResolved | null | undefined if (tsconfigCache.has(cacheKey)) { tsConfig = tsconfigCache.get(cacheKey)! } else { tsconfigRootDir = tsconfigRootDir || process.cwd() let tsconfigResult: TsConfigResult | null | undefined + try { if (project) { const projects = Array.isArray(project) ? project : [project] for (const project of projects) { tsconfigResult = getTsconfig( project === true ? context.filename : path.resolve(tsconfigRootDir, project), ) if (tsconfigResult) { break } } } else { tsconfigResult = getTsconfig(tsconfigRootDir) } tsConfig = tsconfigResult?.config tsconfigCache.set(cacheKey, tsConfig) + } catch (error) { + // Log the error but don't crash + console.error('Error loading tsconfig:', error) + tsConfig = undefined + tsconfigCache.set(cacheKey, tsConfig) + } } return tsConfig }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
.changeset/four-pumas-leave.md(1 hunks)resolvers/README.md(8 hunks)src/types.ts(3 hunks)src/utils/export-map.ts(5 hunks)src/utils/legacy-resolver-settings.ts(3 hunks)src/utils/resolve.ts(7 hunks)test/fixtures/foo-bar-resolver-no-version.js(1 hunks)test/fixtures/foo-bar-resolver-v1.js(1 hunks)test/fixtures/foo-bar-resolver-v2.js(1 hunks)test/fixtures/foo-bar-resolver-v3.js(1 hunks)test/node-resolver.spec.ts(2 hunks)test/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- .changeset/four-pumas-leave.md
- test/utils.ts
- test/fixtures/foo-bar-resolver-v1.js
- test/node-resolver.spec.ts
- test/fixtures/foo-bar-resolver-v3.js
- test/fixtures/foo-bar-resolver-no-version.js
- test/fixtures/foo-bar-resolver-v2.js
- src/utils/legacy-resolver-settings.ts
- src/utils/resolve.ts
- src/types.ts
- resolvers/README.md
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/utils/export-map.ts (3)
198-204: LGTM: Options pattern implementation with lazy loading.The lazy-loaded tsconfig getter in the options object is an excellent pattern for performance optimization. It ensures the tsconfig is only loaded when actually needed.
215-219: LGTM: Good refactoring ofisEsModuleInteropTrueto use the centralized tsconfig.This change consolidates the logic that was previously inline, improving maintainability and consistency.
1164-1164: LGTM: Added requiredcwdproperty to child context.Correctly propagates the current working directory from the parent context, which is essential for the resolver options pattern.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed bc5ac1b in 1 minute and 57 seconds. Click for details.
- Reviewed
588lines of code in12files - Skipped
0files when reviewing. - Skipped posting
16draft 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/four-pumas-leave.md:2
- Draft comment:
Ensure the changeset entry (version/detail) follows the project conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. resolvers/README.md:12
- Draft comment:
The documentation for the newoptionsparameter is clear; consider adding a concrete example of the options object (especially fortsconfig) to further assist users. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/types.ts:57
- Draft comment:
The newResolveOptionsinterface is defined correctly. Consider adding inline comments explaining each property (especiallycontext) for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/utils/export-map.ts:198
- Draft comment:
Lazy evaluation oftsconfigvia a getter is well implemented; ensure that caching behavior does not lead to stale configuration in long-running processes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/utils/legacy-resolver-settings.ts:32
- Draft comment:
The integration of the newoptionsargument in legacy resolver functions looks good; verify that using the fourth argument as undefined remains unambiguous. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/utils/resolve.ts:289
- Draft comment:
In importXResolverCompat, the legacy resolver is wrapped to match v3 by passing the newoptionsparameter. Confirm that the getter fortsconfig(lines 297–300) returns the expected configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. test/fixtures/foo-bar-resolver-no-version.js:12
- Draft comment:
The assertions verify that the legacy resolver receives an undefined fourth argument and a properly populated options.context. This helps maintain backward compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. test/fixtures/foo-bar-resolver-v1.js:12
- Draft comment:
The v1 resolver test correctly asserts that the optional 4th parameter is undefined and that options.context is present. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. test/fixtures/foo-bar-resolver-v2.js:12
- Draft comment:
The v2 resolver test now asserts the presence ofoptions.context.cwd, ensuring the new parameter is handled correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
The v3 resolver test appropriately checks foroptions.context.cwdin the new resolver interface. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. test/node-resolver.spec.ts:32
- Draft comment:
The test passes a valid context (via testContext()) to the resolver. Snapshot testing confirms resolution behavior remains intact. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. resolvers/README.md:14
- Draft comment:
Typo/anchor consistency: The anchor link for[options.context]is given as#optionscontext. If a hyphenated anchor (e.g.,#options-context) is intended for clarity and consistency with similar links, please update it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is about a newly added section in the diff. The current anchor link works correctly - in markdown, anchors are automatically generated by lowercasing and removing punctuation. Adding hyphens would actually break the link since the target section uses the non-hyphenated version. The comment is suggesting a change that would make things worse. Could there be a broader consistency issue across the document that I'm missing? Should all anchors follow a consistent style? Looking at other anchors in the document like #interfaceversion-number, the style is inconsistent, but that's actually fine - markdown automatically handles both formats. The working links are more important than stylistic consistency. Delete this comment as it suggests a change that would break functionality, and anchor formatting is handled automatically by markdown.
13. resolvers/README.md:15
- Draft comment:
Typo/anchor consistency: The anchor link for[options.tsconfig]is given as#optionstsconfig. Consider updating it (e.g., to#options-tsconfig) if that is more consistent with the expected format. - 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 documentation change that would improve consistency and usability. However, looking at the rules, we should not comment on purely informative changes that don't affect functionality. The link still works either way, and this is just a style preference. The inconsistent anchor format could make the documentation harder to navigate and maintain in the future. Bad documentation can be a real issue. While documentation quality matters, this is a minor formatting issue that doesn't prevent users from using the documentation. The link still functions correctly. This comment should be removed as it's purely about documentation formatting and doesn't affect functionality or require code changes.
14. resolvers/README.md:230
- Draft comment:
Typographical error: consider changing 'For compatibility reason, the 4th argument is always passed asundefined.' to 'For compatibility reasons, the 4th argument is always passed asundefined.' - 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% While the grammatical correction is technically correct, it's an extremely minor change that doesn't affect understanding. The documentation is still perfectly clear with either singular or plural. This feels like unnecessary nitpicking that adds noise to the PR review process. The grammatical error could potentially make the documentation look less professional. Some might argue that maintaining high quality even in small details is important. While professionalism is important, this change is so minor that the cost of the review comment outweighs the benefit. The current wording is not incorrect enough to justify a comment. Delete this comment as it's too minor and doesn't materially improve the documentation.
15. resolvers/README.md:247
- Draft comment:
Typographical note: The function signature now includes a new parameter named 'options', but the function body still calls 'opts(file, config)'. Please verify if this is a typo and should be updated to use 'options'. - 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 documentation file showing example code. Theopts()function is likely a helper function that processes the file and config parameters, which is separate from the new options parameter. The code is showing backward compatibility where v2 resolvers can optionally accept the new options parameter. There's no actual typo here - it's working as intended. I could be wrong about theopts()function's purpose since its implementation isn't shown. Maybe there is a real issue here. Even if I'm wrong aboutopts(), this is example code in documentation meant to illustrate the interface. The important part is showing the parameter signature, not the implementation details. This comment should be deleted as it raises a non-issue about implementation details in example documentation code.
16. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Typographical note: The error message 'thecontext.cwdmust be presented' might be clearer as 'thecontext.cwdmust be present' or 'provided'. - 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% While the suggested change would improve the grammar, this is a test fixture file and the error message is not user-facing. The current message, while not perfect English, is still clear enough to understand the intent. Making such a minor grammatical change in test code doesn't provide significant value. The improved grammar could make the code more professional and easier to understand for non-native English speakers. Error messages, even in tests, are a form of documentation. However, the benefit is very minor, and we should avoid nitpicking on test files where the message serves its purpose adequately. Delete this comment as it suggests a trivial grammatical improvement to a test fixture that doesn't meaningfully impact code quality or functionality.
Workflow ID: wflow_8IfgI5AQFRHGqrdt
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
bc5ac1b to
1b2df1a
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 1b2df1a in 1 minute and 57 seconds. Click for details.
- Reviewed
588lines of code in12files - Skipped
0files when reviewing. - Skipped posting
9draft 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. resolvers/README.md:12
- Draft comment:
The newoptionsargument documentation is clear. Consider adding an explicit example showing howcontextandtsconfigare used, especially for legacy resolver behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The documentation already provides links to the type definitions for both context and tsconfig. It also shows a complete example of a resolver implementation. However, these new parameters are marked as only available in version 4.13.0+ and may be unfamiliar to users. An example could help clarify their usage. The comment may be too speculative - we don't know if examples are actually needed since the types are documented. The existing example may be sufficient. While the types are documented, these are new parameters that change how resolvers work. Concrete examples often help developers understand usage better than just type definitions. The comment suggests a valid documentation improvement for new functionality, but it's more of a nice-to-have enhancement than a required change.
2. src/utils/export-map.ts:198
- Draft comment:
The lazy getter fortsconfigis used here. Consider caching its result to avoid multiple evaluations iftsconfig()is invoked repeatedly. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. src/utils/legacy-resolver-settings.ts:91
- Draft comment:
The compatibility logic using the 4th argument (always undefined) is clear. Ensure that documentation in both README and inline comments remains synchronized with this implementation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that documentation is synchronized with the code logic. This falls under the category of asking the author to ensure something is done, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.
4. src/utils/resolve.ts:172
- Draft comment:
The new branch for resolvers using the v3 interface in fullResolve is well integrated. Consider wrapping the call to resolver.resolve in try/catch to handle any unexpected errors and report them gracefully. - 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 code already has error handling in place at a higher level that catches and reports errors gracefully. Adding another try/catch would be redundant since any errors will be caught by the outer try/catch. The resolver has also been validated before being called. The current error handling approach seems appropriate and well-structured. I could be wrong about whether the higher-level error handling is sufficient - there might be specific errors from resolver.resolve() that need special handling at this level. The higher-level error handling appears comprehensive, properly reporting errors to the ESLint context. There's no clear evidence that additional error handling would provide any benefit. The comment should be removed since error handling is already properly implemented at a higher level in the code.
5. src/utils/resolve.ts:285
- Draft comment:
In importXResolverCompat, the legacy resolver compatibility wrapper is implemented correctly. It might be beneficial to reconsider how non-standard errors from legacy resolvers are handled. - 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. test/fixtures/foo-bar-resolver-no-version.js:12
- Draft comment:
The asserts check that the 4th argument is undefined and thatoptions.context.cwdis present. For consistency, ensure the messaging is uniform across all resolver tests. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. test/fixtures/foo-bar-resolver-v1.js:12
- Draft comment:
In this v1 test, the assert message refers to 'thecontextmust be present'. For consistency with other tests (e.g. v2), consider asserting thatoptions.context.cwdis present. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. resolvers/README.md:230
- Draft comment:
Typo: The phrase "For compatibility reason" should be corrected to "For compatibility reasons". - 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% While the comment is technically correct about the grammar, it's an extremely minor issue that doesn't affect comprehension. The documentation's main purpose is to convey technical information, which it does successfully even with this small grammatical error. This kind of nitpicky grammar correction doesn't meet the bar for "clearly a code change required". The grammar error could potentially make the documentation look less professional. Some might argue that maintaining high quality even in small details is important for documentation. While polish is good, this level of nitpicking creates noise and could distract from more important issues. The meaning is perfectly clear despite the minor grammar issue. Delete this comment as it's too minor and doesn't meet the bar for "clearly a code change required". It's a purely cosmetic grammar fix that doesn't impact understanding.
9. resolvers/README.md:247
- Draft comment:
Typo: It appears that the identifier 'opts' is used, but the function signature now includes a parameter named 'options'. Consider replacing 'opts' with 'options' if that was the intended variable. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_5VXPPLdIZO2pyyXy
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
1b2df1a to
62f2d83
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 62f2d83 in 1 minute and 53 seconds. Click for details.
- Reviewed
588lines of code in12files - Skipped
0files when reviewing. - Skipped posting
16draft 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/four-pumas-leave.md:1
- Draft comment:
Changelog file is concise and correctly marks a minor release. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. resolvers/README.md:12
- Draft comment:
Documentation now includes the new 'options' argument with detailed examples; ensure consistency with resolver implementations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/types.ts:57
- Draft comment:
New type 'ResolveOptions' now mandates a 'context' property; consider adding inline comments for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/utils/export-map.ts:1151
- Draft comment:
childContext now includes 'cwd' from context which is essential for tsconfig resolution; verify lazy evaluation does not trigger redundant computation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/utils/export-map.ts:290
- Draft comment:
remotePath passes a lazy tsconfig getter via options; ensure this lazy pattern consistently prevents duplicate computations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/utils/legacy-resolver-settings.ts:90
- Draft comment:
Legacy resolver functions now pass a unified 'options' object; this maintains backward compatibility while adding context. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/utils/resolve.ts:172
- Draft comment:
fullResolve correctly passes the 'options' (with context and extra) to new resolvers and falls back to legacy resolvers appropriately. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/utils/resolve.ts:285
- Draft comment:
importXResolverCompat wraps legacy resolvers using the new signature; ensure the tsconfig getter is correctly passed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. test/fixtures/foo-bar-resolver-no-version.js:12
- Draft comment:
Test fixture asserts that the 4th argument remains undefined and that options.context.cwd is present, ensuring compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. test/fixtures/foo-bar-resolver-v1.js:12
- Draft comment:
Test ensures legacy resolver v1 function receives the new options argument while keeping the 4th argument undefined. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. test/fixtures/foo-bar-resolver-v2.js:12
- Draft comment:
Test for resolver v2 correctly validates the new argument order, with proper assertions on the options argument. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Test fixture for resolver v3 now verifies that options.context is correctly received; the new API is validated here. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. test/node-resolver.spec.ts:31
- Draft comment:
Node resolver tests now pass a context object via options; this aligns with the updated resolver API. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
14. test/utils.ts:155
- Draft comment:
testContext now includes the 'cwd' property, ensuring all resolver functions receive required context. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
15. resolvers/README.md:230
- Draft comment:
Typo: Consider changing "For compatibility reason" to "For compatibility reasons" for grammatical correctness. - 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% While the comment is technically correct, our rules state that we should not make purely informative comments that don't require code changes. Grammar fixes in documentation, while nice to have, are very minor and don't impact functionality. The meaning is still clear even with the minor grammatical error. The documentation is user-facing, so maintaining professional and correct grammar could be considered important for project quality. However, our rules explicitly state not to make purely informative comments. Grammar fixes in documentation are not critical enough to warrant interrupting the PR review process. Delete this comment as it's a minor grammatical suggestion that doesn't impact functionality and violates our rule about purely informative comments.
16. resolvers/README.md:247
- Draft comment:
Typographical issue: The code callsopts(file, config)but the updated function signature now includes anoptionsparameter. It appears that there might be a typo or oversight here—shouldoptsbe replaced withoptions(or should the function call be updated accordingly)? - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_bx1ZF1mTHkS0TWTc
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
62f2d83 to
928ad53
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 928ad53 in 2 minutes and 13 seconds. Click for details.
- Reviewed
595lines of code in12files - Skipped
0files when reviewing. - Skipped posting
20draft 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. resolvers/README.md:11
- Draft comment:
New documentation for the 'options' argument is added. Verify that the reference links ([ChildContext] and [TsConfigJsonResolved]) are correct and accessible. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. resolvers/README.md:48
- Draft comment:
The new v3 resolver signature now includes anoptionsargument. Consider providing an example that also demonstrates usage ofoptions.tsconfig. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
3. src/types.ts:147
- Draft comment:
ChildContext has been updated to include a required 'cwd' and to make 'filename' non-optional. Ensure that all consumers are updated to provide these properties. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/types.ts:57
- Draft comment:
New types ResolveOptionsExtra and ResolveOptions correctly encapsulate the tsconfig option; double-check backward compatibility with legacy usage. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
5. src/utils/export-map.ts:93
- Draft comment:
The new getTsconfigWithContext function uses caching for tsconfig. Consider adding error handling in case getTsconfig fails or returns an unexpected value. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/utils/export-map.ts:288
- Draft comment:
Passing a lazy getter for tsconfig via the extra options to the relative() function looks correct; verify that lazy evaluation does not introduce memory retention issues. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
7. src/utils/legacy-resolver-settings.ts:28
- Draft comment:
LegacyResolver types have been updated to include the new options parameter. Ensure that all legacy resolver implementations are adapted to this new signature. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
8. src/utils/legacy-resolver-settings.ts:90
- Draft comment:
In resolveWithLegacyResolver the constructed options object ({ context, ...extra }) is passed to legacy resolvers. Ensure legacy resolvers properly handle the new options argument. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
9. src/utils/resolve.ts:172
- Draft comment:
fullResolve now passes { context, ...extra } to the resolver.resolve call. Confirm that the memoized settings hash (used in cacheKey) remains consistent with changes in settings. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
10. src/utils/resolve.ts:290
- Draft comment:
importXResolverCompat correctly wraps legacy resolvers using the new resolver interface. Verify that the getter for tsconfig in the options works as intended. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
11. test/fixtures/foo-bar-resolver-no-version.js:12
- Draft comment:
Test fixture asserts that the 4th argument is undefined and that options.context.cwd exists. This validates the new resolver interface for resolvers without explicit version. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. test/fixtures/foo-bar-resolver-v1.js:12
- Draft comment:
Test for v1 resolver now passes the new options parameter. Ensure backward compatibility is maintained for interfaceVersion 1. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. test/fixtures/foo-bar-resolver-v2.js:12
- Draft comment:
Test for v2 resolver confirms that options.context.cwd is provided. Verify that legacy v2 resolvers handle the additional options argument correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
14. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Test for v3 resolver asserts that options.context.cwd is available. This confirms alignment with the new v3 resolver interface. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
15. test/node-resolver.spec.ts:31
- Draft comment:
Updated test now passes a 'context' parameter to resolver.resolve, validating the new interface integration in the node-resolver. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
16. test/utils.ts:156
- Draft comment:
The testContext function now includes a 'cwd' property. Confirm that this change is in sync with the updated ChildContext type. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
17. resolvers/README.md:14
- Draft comment:
Typographical suggestion: The anchor foroptions.contextmight be intended to have a hyphen (e.g.,#options-context) for clarity and consistency with typical markdown heading conventions. - 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 current anchor link actually works fine - it points to the correct section. While adding hyphens might be more conventional, this is a minor stylistic suggestion that doesn't affect functionality. The comment is about formatting rather than substantive content. Per the rules, we should avoid purely informative comments that don't require clear code changes. The suggestion could improve consistency with common markdown practices. Bad anchor formatting could potentially cause issues with some markdown parsers. While consistent formatting is good, the current anchor works fine and this is an extremely minor stylistic issue. The rules explicitly say not to make purely informative comments. Delete this comment as it suggests a minor formatting change that isn't strictly necessary and doesn't affect functionality.
18. resolvers/README.md:15
- Draft comment:
Typographical suggestion: The anchor foroptions.tsconfigmight be intended to include a hyphen (e.g.,#options-tsconfig) to improve clarity and consistency. - 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 about a minor formatting detail in a documentation file. While technically correct that hyphenated anchors are more common, the current anchor works fine - GitHub Markdown automatically handles both formats. This is a very minor stylistic suggestion that doesn't impact functionality. The comment is technically accurate and suggests a more standard format. Consistency in documentation is valuable. However, this is an extremely minor stylistic change that doesn't affect functionality or readability. The current format works perfectly fine. This comment should be removed as it suggests a trivial change that doesn't meaningfully improve the documentation.
19. resolvers/README.md:230
- Draft comment:
Typo: Consider changing "For compatibility reason" to "For compatibility reasons" for proper subject-verb agreement. - 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% While the comment is technically correct about the grammar, it's an extremely minor issue in documentation. The meaning is perfectly clear either way. The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category - it's not suggesting any meaningful code changes. The grammar error could make the documentation look slightly unprofessional. Documentation quality is important for open source projects. While documentation quality matters, this is such a minor grammatical issue that it doesn't meaningfully impact readability or understanding. The review process should focus on more substantial issues. Delete this comment as it's too minor and doesn't suggest any meaningful changes that would improve the code or documentation significantly.
20. resolvers/README.md:247
- Draft comment:
Typographical error: The code callsopts(file, config)but the function signature includes a parameter namedoptionsthat should likely be used here. Please updateopts(file, config)to useoptionsor adjust accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a documentation file showing an example of a v2 resolver. Theopts()function call is part of that example code, likely from a real implementation. Theoptionsparameter was added later for v3 compatibility but is optional. The example is showing historical code, so it's correct as-is. The comment misunderstands the purpose of this section. Could this be a real issue where the example should be updated to show modern best practices? Maybe showing outdated patterns is confusing to readers? No - this section explicitly documents the v2 interface and shows a historical example. It's important to show the original code for compatibility understanding. The v3 example earlier in the doc shows the modern approach. The comment should be deleted. It misunderstands that this is intentionally showing historical v2 code, and theoptionsparameter is an optional later addition for compatibility.
Workflow ID: wflow_h7kxM8OInkH3E184
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
928ad53 to
8887aa6
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8887aa6 in 2 minutes and 59 seconds. Click for details.
- Reviewed
602lines of code in12files - Skipped
0files when reviewing. - Skipped posting
15draft 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/four-pumas-leave.md:2
- Draft comment:
Changelog entry is clear and concise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. resolvers/README.md:13
- Draft comment:
Ensure the newoptionsargument documentation is consistent and links ([ChildContext], [TsConfigJsonResolved]) are up‐to‐date. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/types.ts:57
- Draft comment:
New interfaces (ResolveOptionsExtra and ResolveOptions) look good; confirm that both ChildContext and RuleContext are always provided. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/utils/export-map.ts:196
- Draft comment:
Lazy evaluation for tsconfig is neat; consider caching the computed tsconfig to minimize repeated lookups. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/utils/legacy-resolver-settings.ts:90
- Draft comment:
Legacy resolver types now include the new options; verify that the 4th argument remains undefined for backward compatibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/utils/resolve.ts:170
- Draft comment:
New resolver calls properly pass { ...extra, context }; ensure downstream resolvers correctly use the lazy tsconfig getter. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. test/fixtures/foo-bar-resolver-no-version.js:12
- Draft comment:
Test asserts 4th arg is undefined and options.context includes cwd—good for backward compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. test/fixtures/foo-bar-resolver-v1.js:12
- Draft comment:
Legacy v1 resolver test correctly checks for the undefined 4th arg and context presence. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. test/fixtures/foo-bar-resolver-v2.js:12
- Draft comment:
v2 resolver test validates 4th argument is undefined and context.cwd exists; maintains legacy compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
v3 resolver test properly asserts that options.context contains cwd. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. test/node-resolver.spec.ts:31
- Draft comment:
Test updates pass a proper context via testContext; ensure it supplies all required fields (e.g., cwd). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. test/utils.ts:156
- Draft comment:
testContext now returns an object with 'cwd'; verify no missing required fields. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. resolvers/README.md:14
- Draft comment:
The anchor foroptions.contextis written as#optionscontext. For consistency with other anchors (e.g.,#interfaceversion-number), consider using a hyphen as in#options-context. - 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 documentation formatting suggestion. While consistency is good, this is a very minor issue. The anchor links will work either way. The comment isn't about any functional changes or code quality issues. Looking at the rules, we should not make purely informative comments or comments about unimportant issues. The inconsistency in anchor formatting could make the documentation harder to maintain in the future. Bad documentation can be a real problem. While documentation is important, this is an extremely minor formatting issue that doesn't affect functionality or readability. The rules specifically say not to make comments that are unimportant. Delete this comment as it's too minor of an issue and doesn't affect functionality.
14. resolvers/README.md:15
- Draft comment:
The anchor foroptions.tsconfigis written as#optionstsconfig. For consistency with other anchors, consider using a hyphen as in#options-tsconfig. - 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 very minor formatting suggestion about markdown anchor links. It doesn't affect functionality since both formats would work. The current format is not clearly wrong or inconsistent with the rest of the document. This seems like the kind of nitpicky comment we should avoid. The comment does point out an inconsistency in the document's formatting. Having consistent formatting could make the document more professional and maintainable. While consistency is good, this is too minor of an issue to warrant a comment. The current format works fine and changing it provides minimal value. This comment should be deleted as it's too minor and doesn't highlight a real issue that needs fixing.
15. resolvers/README.md:230
- Draft comment:
Typographical suggestion: Consider changing "For compatibility reason, the 4th argument is always passed asundefined." to "For compatibility reasons, the 4th argument is always passed asundefined." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While technically correct, this is an extremely minor grammatical fix. The original meaning is perfectly clear. The rules specifically state not to make comments that are obvious or unimportant. This seems to fall into that category. Documentation readability is important, but this change is too minor to warrant a PR comment. Perhaps poor documentation could lead to developer confusion, and we should maintain high standards for technical writing? While documentation quality matters, this tiny grammatical fix doesn't meaningfully improve clarity or understanding. The cost of the PR review interaction outweighs the minimal benefit. This comment should be deleted as it violates the rule about not making obvious or unimportant comments. The grammatical fix is too minor to warrant a PR comment.
Workflow ID: wflow_UaTOM4DKaKsBABxv
You can customize
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/types.ts (1)
140-156:⚠️ Potential issueAdd
cwd,filename, andphysicalFilenametoRuleContextto keep type-safety intact
childContext()(utils/export-map.ts) andresolve()(utils/resolve.ts) readcontext.cwd,context.filename, andcontext.physicalFilename.
Those properties are now required onChildContext, but they are still missing from our localRuleContextaugmentation.
Depending on the version of@typescript-eslint/utils, the upstreamRuleContextmay or may not contain them, so compilation can break understrictprojects.export interface RuleContext< TMessageIds extends string = string, TOptions extends readonly unknown[] = readonly unknown[], > extends Omit<TSESLint.RuleContext<TMessageIds, TOptions>, 'settings'> { settings: PluginSettings + /** Current working directory of this ESLint run */ + cwd: string + /** Logical file name after preprocessors */ + filename: string + /** Physical file name on disk */ + physicalFilename: string }This keeps the contract symmetric with
ChildContextand prevents downstream--noImplicitAny/--strictNullCheckserrors.
♻️ Duplicate comments (3)
test/fixtures/foo-bar-resolver-no-version.js (1)
12-13:⚠️ Potential issueMissing return statement after assertions
The assertions for parameter validation are good, but the original
return undefinedstatement should be preserved to maintain the expected behavior of the resolver.assert.ok(!_, 'the 4th argument must be undefined') assert.ok(options.context.cwd, 'the `context.cwd` must be present') + return undefinedtest/fixtures/foo-bar-resolver-v1.js (1)
12-13:⚠️ Potential issueMissing return statement after assertions
The assertions for parameter validation are good, but the original
return undefinedstatement should be preserved to maintain the expected behavior of the resolver.assert.ok(!_, 'the 4th argument must be undefined') assert.ok(options.context.cwd, 'the `context` must be present') + return undefinedresolvers/README.md (1)
13-16: Fix broken Markdown anchors for nestedoptionsdocsThe anchors render as
#optionscontext/#optionstsconfig, but typical Markdown-it style
expects a hyphen:#options-context/#options-tsconfig.
Broken anchors make the new documentation harder to navigate.- - [`options.context`](#optionscontext) - - [`options.tsconfig`](#optionstsconfig) + - [`options.context`](#options-context) + - [`options.tsconfig`](#options-tsconfig)
🧹 Nitpick comments (4)
src/types.ts (1)
57-64: Document the newResolveOptions*types for future maintainersThe extra
optionsobject is central to the new resolver API. A short JSDoc block on
ResolveOptionsExtraandResolveOptions—explaining whytsconfigis optional and
whatcontextis expected to contain—would save readers a round-trip to the README.No code change required; only docs.
resolvers/README.md (1)
25-27: Consider updating the v2 resolver signature block for clarityYou describe the extra “4th = undefined” and “5th = options” parameters, but the code
block above still shows the three-parameter v2 signature. Either update the signature
or add an explicit note that only callers receive five parameters while the formal
type remains unchanged. This will avoid confusion for authors referencing the spec.src/utils/resolve.ts (1)
241-253: Expose the newextraargument to callers for maximum flexibility
resolve()now accepts an optionalextrabutrelative()forwardsextraeven when
undefined, which is fine. Consider documenting the public helper in JSDoc so rule
authors know they can supplytsconfiglazily:/** * @param extra - e.g. `{ tsconfig: myTsConfig }` */ export function resolve(…)This aligns the runtime helper with the README changes.
src/utils/export-map.ts (1)
93-123:getTsconfigWithContext– cache key can leak absolute project paths
stableHash({ tsconfigRootDir, project })hashes full project strings.
WhenparserOptions.projectcontains absolute paths, you end up with one cache entry
per machine / CI path, defeating sharing and increasing memory.Suggestion: normalise
projectentries to relative paths before hashing:- const cacheKey = stableHash({ tsconfigRootDir, project }) + const normProject = Array.isArray(project) + ? project.map(p => typeof p === 'string' ? path.resolve(tsconfigRootDir, p) : p) + : project + const cacheKey = stableHash({ tsconfigRootDir, project: normProject })This makes the cache portable across environments.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-107: src/utils/export-map.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 109-110: src/utils/export-map.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 113-113: src/utils/export-map.ts#L113
Added line #L113 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
.changeset/four-pumas-leave.md(1 hunks)resolvers/README.md(8 hunks)src/types.ts(3 hunks)src/utils/export-map.ts(5 hunks)src/utils/legacy-resolver-settings.ts(3 hunks)src/utils/resolve.ts(7 hunks)test/fixtures/foo-bar-resolver-no-version.js(1 hunks)test/fixtures/foo-bar-resolver-v1.js(1 hunks)test/fixtures/foo-bar-resolver-v2.js(1 hunks)test/fixtures/foo-bar-resolver-v3.js(1 hunks)test/node-resolver.spec.ts(2 hunks)test/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/node-resolver.spec.ts (1)
test/utils.ts (1)
testContext(155-161)
src/utils/legacy-resolver-settings.ts (1)
src/types.ts (7)
ResolveOptions(61-63)LegacyResolverResolve(23-23)LegacyResolverResolve(24-24)ChildContext(147-157)RuleContext(140-145)ResolveOptionsExtra(57-59)ResolvedResult(98-98)
🪛 GitHub Check: codecov/patch
src/utils/export-map.ts
[warning] 106-107: src/utils/export-map.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 109-110: src/utils/export-map.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 113-113: src/utils/export-map.ts#L113
Added line #L113 was not covered by tests
🔇 Additional comments (22)
.changeset/four-pumas-leave.md (1)
1-5: LGTM - The changeset description is clear and concise.The changeset correctly describes the feature being introduced - adding a new
optionsargument to the resolver with full backward compatibility. This aligns with the PR objectives.test/utils.ts (1)
155-161: Good addition of cwd property to support the new resolver options.The addition of
cwd: FIXTURES_PATHto the test context is appropriate and provides the necessary context property required by the updated resolver interface. This ensures that tests can correctly verify the newoptionsparameter behavior.test/node-resolver.spec.ts (2)
5-6: LGTM - Good import of the testContext function.The import statement for
testContextis correctly added to support the updated resolver interface.
31-33: Well implemented options parameter with context.The resolver call has been properly updated to include the new
optionsparameter with the context object. This change correctly implements the PR's objective of enhancing resolver functions with an options argument.test/fixtures/foo-bar-resolver-v2.js (3)
1-1: Good update to use strict assertions.Updated to use
assert/strictwhich is appropriate for testing the expected behavior of the new parameters.
4-4: Function signature correctly updated with new parameters.The resolver function signature has been properly updated to include the additional parameters (
_andoptions) while maintaining backward compatibility with existing code.
12-13:❌ Incorrect review comment
Consider defensive validation for options parameter.
While the assertions correctly verify the expected behavior, there's no validation to handle cases where
optionsmight be undefined or null. This could cause runtime errors if called from legacy code that doesn't provide the parameter.
🏁 Script executed:
#!/bin/bash # Check if the resolver is used anywhere without providing the options parameter # Look for resolver.resolve calls throughout the codebase echo "Searching for resolver.resolve calls..." rg "resolver\.resolve\(" --type js --type ts -A 3 # Look specifically for places where the resolve function is called with fewer than 3 parameters echo "\nChecking for resolve calls potentially missing options parameter..." ast-grep --pattern "resolve($_, $_, $_)"Length of output: 1193
🏁 Script executed:
#!/bin/bash # Display the test fixture around the relevant lines echo "=== test/fixtures/foo-bar-resolver-v2.js (lines 1–50) ===" sed -n '1,50p' test/fixtures/foo-bar-resolver-v2.js # Search for resolve import and invocation in this fixture echo -e "\n=== resolve usage in foo-bar-resolver-v2.js ===" rg "resolve" -n test/fixtures/foo-bar-resolver-v2.jsLength of output: 1076
Ignore defensive validation for options in this test fixture
The
foo-bar-resolver-v2.jsfile is only used as a version 2 plugin test fixture, and the resolver harness always callsplugin.resolve(modulePath, sourceFile, config, nextResolve, options)so
optionswill never beundefined. No production code invokes this fixture directly, and its assertions are intended to verify the test harness’s behavior, not to harden the fixture itself. You can safely omit additional null/undefined guards here.Likely an incorrect or invalid review comment.
test/fixtures/foo-bar-resolver-v3.js (3)
3-3: Module import added to support assertionsThe addition of
assert/strictis appropriate for the new validation being added to the resolver function.
10-10: New parameter aligned with interface updatesThe resolver function signature has been updated to accept an
optionsparameter, which aligns with the PR objective of adding this parameter to all resolver interfaces while maintaining backward compatibility.
18-18: Assertion validates required context propertyGood validation to ensure that the expected context data is being passed through the new options parameter. This helps verify the resolver is being called correctly with the required context information.
test/fixtures/foo-bar-resolver-no-version.js (2)
1-1: Module import added to support assertionsThe addition of
assert/strictis appropriate for the new validation being added to the resolver function.
4-4: Updated function signature with new parametersThe resolver function signature has been updated to add the fourth parameter
_(placeholder) and fifth parameteroptions, which aligns with the PR objective.test/fixtures/foo-bar-resolver-v1.js (2)
1-1: Module import added to support assertionsThe addition of
assert/strictis appropriate for the new validation being added to the resolver function.
4-4: Updated function signature with new parametersThe resolver function signature has been updated to add the fourth parameter
_(placeholder) and fifth parameteroptions, which aligns with the PR objective.src/utils/legacy-resolver-settings.ts (6)
10-16: Added required type importsThe additional imports for context and options types are necessary for the implementation of the new resolver options parameter.
28-34: Updated LegacyResolverResolveImport type signatureThe type signature has been properly extended to include the new parameters while maintaining backward compatibility through an
undefinedplaceholder parameter.
36-42: Updated LegacyResolverResolve type signatureThe type signature has been properly extended to include the new parameters while maintaining backward compatibility through an
undefinedplaceholder parameter.
83-92: Extended resolveWithLegacyResolver parametersThe function now accepts context and extra options parameters, which are properly combined into a single options object passed to legacy resolvers.
93-95: Updated resolver v2 interface callThe interface version 2 resolver call has been updated to pass the undefined placeholder and options object, maintaining backward compatibility while extending functionality.
97-105: Updated resolveImport call parametersThe resolveImport function call has been properly updated to include the undefined placeholder and options object, maintaining backward compatibility while extending functionality.
src/utils/resolve.ts (1)
172-175: Good catch on placingcontextlast – prevents accidental overwriteSwitching to
{ ...extra, context }ensures a rogueextra.contextcannot shadow the
real ESLint context. 👍src/utils/export-map.ts (1)
1162-1165: Fallback for missing path fields inchildContextAs in
types.ts, copyingcontext.filenameorphysicalFilenamewhen they’re
undefinedpropagates bogus values into caches. Default sensibly or assert their
presence to avoid subtle cache misses.
| @@ -42,7 +47,7 @@ exports.interfaceVersion = 3 | |||
|
|
|||
| ### Required `resolve` | |||
|
|
|||
| Signature: `(source: string, file: string) => { found: boolean, path?: string | null }` | |||
| Signature: `(source: string, file: string, options: { context: ChildContext | RuleContext, tsconfig?: TsConfigJsonResolved }) => { found: boolean, path?: string | null }` | |||
There was a problem hiding this comment.
I don't like the idea of passing an object here. This creates multiple objects per resolve call and is not memory-friendly.
| @@ -212,7 +241,7 @@ Here is most of the [Node resolver] at the time of this writing. It is just a wr | |||
| var resolve = require('resolve/sync') | |||
| var isCoreModule = require('is-core-module') | |||
|
|
|||
| exports.resolve = function (source, file, config) { | |||
| exports.resolve = function (source, file, config, _, options) { | |||
There was a problem hiding this comment.
config and options are easy confusions.
| ##### `undefined` | ||
|
|
||
| For compatibility reason, the 4th argument is always passed as `undefined`. Take [TypeScript resolver](https://github.com/import-js/eslint-import-resolver-typescript/blob/c45039e5c310479c1e178c2180e054380facbadd/src/index.ts#L69) for example. It uses the 4th argument to pass the optional [unrs-resolver] `ResolverFactory` instance to support both `v2` and `v3` interface at the same time. |
There was a problem hiding this comment.
TBH, this undefined feels like bad API design. If we need to add a bandit to the previous design failure, I'd rather introduce a breaking change. Alternatively, we can use some kind of JavaScript context implementation to do this, something like useRuleContext() and useResolvedTsConfig().
There was a problem hiding this comment.
The goal is 100% compatibility at least in this PR, v2 interface has been frozen for a long time, I don't know how a breaking change would work even in a major version.
This is not a design, just how it has to be.
There was a problem hiding this comment.
How are they different with the lazy getter? I don't want over-enginering here. Or would you like to write some pseudo codes here?
There was a problem hiding this comment.
The goal is 100% compatibility at least in this PR, v2 interface has been frozen for a long time, I don't know how a breaking change would work even in a major version.
This is not a design, just how it has to be.
What I am proposing here is that we implement a context provider (it doesn't mean rule context) that lives outside of the full resolve closure scope. The resolver would be doing this instead:
import { resolverUtils } from 'eslint-plugin-import-x';
// this importee can vary
export const resolver = {
interfaceVersion: 3,
resolve(StillOnly, TwoArguments) {
if ('useRuleContext' in resolveUtils) {
const context = resolverUtils?.useRuleContext();
}
const tsconfig = resolverUtils?.useResolvedTsConfig();
}
}
// Heck, even the old v2 interface can benefits!
exports.resolveImport = (StillOnly, TwoArguments) => {
const context = useRuleContext();
const tsconfig = useResolvedTsConfig();
};There was a problem hiding this comment.
How about push a commit to this PR/branch?
| assert.ok(!_, 'the 4th argument must be undefined') | ||
| assert.ok(options.context.cwd, 'the `context.cwd` must be present') |
There was a problem hiding this comment.
We are still using jest, use expect style here.
There was a problem hiding this comment.
Hmm, it's a resolver implementation, I don't know would it be suitable to use jest API here.
There was a problem hiding this comment.
What I mean is that we can simply throw TypeError here, and use expect#toThrow in the unit test cases.
There was a problem hiding this comment.
There is no error thrown, the assertions pass, if they fail, the jest test case fails.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2f78cfd in 1 minute and 53 seconds. Click for details.
- Reviewed
173lines of code in7files - Skipped
0files when reviewing. - Skipped posting
9draft 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. .github/workflows/ci.yml:66
- Draft comment:
Updated Codecov action ref to v5.4.3. Verify that the new version is fully compatible. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/utils/export-map.ts:97
- Draft comment:
Switching to an array for stableHash input enforces a consistent key ordering. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the effect of switching to an array forstableHashinput. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
3. src/utils/export-map.ts:195
- Draft comment:
Use defineLazyProperty for lazy tsconfig injection in ExportMap.get for clarity and consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. src/utils/export-map.ts:286
- Draft comment:
Refactored remotePath to use defineLazyProperty for lazy tsconfig evaluation, ensuring consistent behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what was done in the code without providing any actionable feedback or suggestions. It does not ask for confirmation or suggest any changes.
5. src/utils/resolve.ts:295
- Draft comment:
Adopt defineLazyProperty in importXResolverCompat for lazy tsconfig injection to match the new resolver signature. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. test/fixtures/foo-bar-resolver-no-version.js:14
- Draft comment:
Test updated: conditionally asserts tsconfig presence based on parserOptions.project. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating that a test was updated to conditionally assert the presence oftsconfigbased onparserOptions.project. It does not provide any actionable feedback or suggestions for improvement.
7. test/fixtures/foo-bar-resolver-v1.js:14
- Draft comment:
Test updated: validates tsconfig based on parserOptions.project presence. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, describing what was done in the test update. It does not provide any actionable feedback or suggestions for improvement.
8. test/fixtures/foo-bar-resolver-v2.js:14
- Draft comment:
Test updated: verifies tsconfig property based on parserOptions.project condition. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that a test was updated to verify a specific property. It does not provide any actionable feedback or suggestions for improvement.
9. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Test updated for resolver v3: asserts that tsconfig is provided (or absent) in accordance with parserOptions.project. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, describing what the test update does. It doesn't provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_wkGHOQdJ0rUqKVWL
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
d876f18 to
780a1d8
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 780a1d8 in 53 seconds. Click for details.
- Reviewed
173lines of code in7files - Skipped
0files when reviewing. - Skipped posting
9draft 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. .github/workflows/ci.yml:65
- Draft comment:
Updated Codecov action to v5.4.3. Confirm that the new version works well with our CI configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/utils/export-map.ts:97
- Draft comment:
Changed cacheKey generation from an object to an array for stableHash. Ensure this new key format maintains cache compatibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/utils/export-map.ts:195
- Draft comment:
Refactored the resolver call to use defineLazyProperty for tsconfig. Verify that lazy evaluation behaves as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/utils/export-map.ts:288
- Draft comment:
Updated remotePath helper to use defineLazyProperty for tsconfig. Confirm this aligns with the new resolver options. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/utils/resolve.ts:298
- Draft comment:
Replaced the inline getter for tsconfig with defineLazyProperty in importXResolverCompat for consistency. Validate that the lazy evaluation of tsconfig works across resolver versions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. test/fixtures/foo-bar-resolver-no-version.js:14
- Draft comment:
Test updated to assert that tsconfig is present (or absent) based on parserOptions.project. This confirms the new resolver behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. test/fixtures/foo-bar-resolver-v1.js:14
- Draft comment:
Test updated to check that tsconfig is provided when parserOptions.project is defined, ensuring compatibility with the new resolver options. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. test/fixtures/foo-bar-resolver-v2.js:14
- Draft comment:
Test now verifies that tsconfig is appropriately passed when parserOptions.project exists, aligning with the new implementation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. test/fixtures/foo-bar-resolver-v3.js:18
- Draft comment:
Test updated to ensure that tsconfig is provided to the resolver when parserOptions.project is set, matching the new resolver options contract. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_nQcxDrwgy4zHyWk5
You can customize
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/utils/export-map.ts (1)
106-107: Consider adding test coverage for edge cases in project resolution.The static analysis indicates that several edge cases in the project resolution logic are not covered by tests, particularly around handling multiple projects and fallback scenarios.
While these edge cases may be difficult to test in isolation, consider adding test cases that exercise:
- Multiple project configurations where the first project fails to resolve
- Boolean project values that trigger physicalFilename fallback
- Cache hit scenarios with different project configurations
Would you like me to help generate test cases for these scenarios?
Also applies to: 109-110, 113-113
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-107: src/utils/export-map.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci.yml(1 hunks)src/utils/export-map.ts(5 hunks)src/utils/resolve.ts(7 hunks)test/fixtures/foo-bar-resolver-no-version.js(1 hunks)test/fixtures/foo-bar-resolver-v1.js(1 hunks)test/fixtures/foo-bar-resolver-v2.js(1 hunks)test/fixtures/foo-bar-resolver-v3.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/resolve.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/utils/export-map.ts
[warning] 106-107: src/utils/export-map.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 109-110: src/utils/export-map.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 113-113: src/utils/export-map.ts#L113
Added line #L113 was not covered by tests
test/fixtures/foo-bar-resolver-no-version.js
[warning] 15-15: test/fixtures/foo-bar-resolver-no-version.js#L15
Added line #L15 was not covered by tests
test/fixtures/foo-bar-resolver-v1.js
[warning] 15-15: test/fixtures/foo-bar-resolver-v1.js#L15
Added line #L15 was not covered by tests
test/fixtures/foo-bar-resolver-v2.js
[warning] 15-15: test/fixtures/foo-bar-resolver-v2.js#L15
Added line #L15 was not covered by tests
test/fixtures/foo-bar-resolver-v3.js
[warning] 20-20: test/fixtures/foo-bar-resolver-v3.js#L20
Added line #L20 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (94)
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
🔇 Additional comments (17)
test/fixtures/foo-bar-resolver-v2.js (3)
1-1: LGTM! Proper import for assertions.The addition of
assert/strictimport is appropriate for the validation logic being added.
4-4: Function signature correctly updated for v2 resolver API.The signature properly includes the
_placeholder parameter for backward compatibility and the newoptionsparameter. This maintains the v2 resolver interface contract.
12-13: Solid validation of the new API parameters.The assertions correctly validate that the 4th parameter is undefined (ensuring backward compatibility) and that the options contain the required context with cwd property.
test/fixtures/foo-bar-resolver-no-version.js (3)
1-1: LGTM! Consistent import pattern.Adding
assert/strictimport aligns with the validation pattern used across all resolver fixtures.
4-4: Function signature properly updated for legacy resolver.The signature correctly adds the
_parameter andoptionsparameter while maintaining theresolveImportfunction name for the no-version resolver interface.
12-24: Assertions validate the new API contract correctly.The validation logic is consistent with other resolver fixtures. Note that the function implicitly returns
undefinedwhen no specific source file matches, which is functionally equivalent to an explicitreturn undefinedstatement.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-15: test/fixtures/foo-bar-resolver-no-version.js#L15
Added line #L15 was not covered by teststest/fixtures/foo-bar-resolver-v3.js (3)
3-3: LGTM! Consistent import pattern.The
assert/strictimport aligns with the validation approach used across all resolver fixtures.
10-10: Correct v3 resolver signature implementation.The function signature properly implements the v3 resolver interface with only 3 parameters (no
_placeholder needed), which is the correct approach for the modern resolver API.
18-18: Solid validation of required context properties.The assertion correctly validates that
options.context.cwdis present, ensuring the resolver receives the necessary context information.test/fixtures/foo-bar-resolver-v1.js (3)
1-1: LGTM! Consistent validation setup.The
assert/strictimport supports the parameter validation logic being added.
4-4: Function signature correctly updated for v1 resolver.The signature properly includes the backward compatibility
_parameter and the newoptionsparameter while maintaining theresolveImportfunction name for v1 interface.
12-13: Proper validation of API contract parameters.The assertions correctly validate the expected parameter structure for the new resolver API. The implicit return of
undefinedis functionally equivalent to an explicit return statement.src/utils/export-map.ts (5)
93-123: LGTM! Well-implemented tsconfig caching with proper edge case handling.The
getTsconfigWithContextfunction provides a clean, centralized approach to TypeScript configuration retrieval with appropriate caching. The implementation correctly:
- Uses array-based cache keys for better performance (addressing SukkaW's feedback)
- Handles both single project strings and project arrays
- Falls back to context.physicalFilename when project is
true- Properly caches results to avoid redundant filesystem operations
The optional chaining in
tsConfig = tsconfigResult?.configensures that nulltsconfigResultyieldsundefinedrather thannull, maintaining type safety.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-107: src/utils/export-map.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 109-110: src/utils/export-map.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 113-113: src/utils/export-map.ts#L113
Added line #L113 was not covered by tests
196-202: Excellent integration of lazy tsconfig loading with resolver options.The modification to
ExportMap.getproperly implements the new resolver interface by passing tsconfig through the options parameter. The use ofdefineLazyPropertyensures efficient lazy evaluation while maintaining clean separation of concerns.
213-217: Clean refactoring to use centralized tsconfig retrieval.The changes consolidate tsconfig access through the new
getTsconfigWithContextfunction, replacing the previous inline implementation. The lazy evaluation ofisEsModuleInteropTruemaintains performance while improving code organization.
289-296: Proper propagation of context and tsconfig to resolution logic.The
remotePathhelper function is correctly updated to pass the additional context and tsconfig parameters through the options argument, maintaining consistency with the new resolver interface throughout the codebase.
1164-1166: Essential context property propagation ensures complete child context.The explicit setting of
cwd,filename, andphysicalFilenameproperties ensures that the child context contains all necessary information for proper resolution, addressing the interface requirements defined in the broader PR changes.
JounQin
force-pushed
the
feat/resolver_options
branch
from
780a1d8 to
f8e571b
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f8e571b in 3 minutes and 35 seconds. Click for details.
- Reviewed
353lines of code in9files - Skipped
0files when reviewing. - Skipped posting
16draft 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. .github/workflows/ci.yml:65
- Draft comment:
Updated Codecov action commit to v5.4.3. Verify that the new version’s interface is compatible with our workflow. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/utils/export-map.ts:97
- Draft comment:
Using an array for stableHash (i.e. [tsconfigRootDir, project]) cleans up potential ordering issues compared to an object. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/utils/export-map.ts:195
- Draft comment:
Refactored resolver usage: replacing inline getter with defineLazyProperty for the 'tsconfig' property ensures lazy evaluation and consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/utils/export-map.ts:1146
- Draft comment:
Exporting childContext (changed from function declaration) improves reusability in other modules. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/utils/resolve.ts:158
- Draft comment:
Using Object.hasOwn(settings, 'import-x/resolver-next') is a modern, cleaner way to check properties. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/utils/resolve.ts:134
- Draft comment:
Including childContext's cacheKey in the full cache key enhances cache uniqueness and prevents collisions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. src/utils/resolve.ts:296
- Draft comment:
In importXResolverCompat, the use of defineLazyProperty for the 'tsconfig' option aligns legacy resolver behavior with the new options argument. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. test/fixtures/foo-bar-resolver-no-version.js:6
- Draft comment:
Test updated to check project setting: returns bar.tsx when parserOptions.project is true and asserts tsconfig presence accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
9. test/fixtures/foo-bar-resolver-v1.js:6
- Draft comment:
Resolver-v1 now validates the 'project' option by ensuring tsconfig is present (or not) as expected. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
10. test/fixtures/foo-bar-resolver-v2.js:6
- Draft comment:
Resolver-v2 also adjusts to return bar.tsx when project is set and performs asserts on options.tsconfig. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
11. test/fixtures/foo-bar-resolver-v3.js:10
- Draft comment:
Resolver-v3 similarly checks project and tsconfig; the update ensures option propagation is consistent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
12. test/utils/resolve.spec.ts:772
- Draft comment:
New tests validate that when languageOptions.parserOptions.project is true, the resolved path switches to bar.tsx. This confirms proper handling of the new options argument. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
13. src/utils/export-map.ts:1148
- Draft comment:
There's a typographical error in the comment. The sentence "don't hold full context object in memory, just grab what we need. also calculate a cacheKey..." should capitalize 'also' (i.e., change it to 'Also') for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical error in a comment, which is not critical to the functionality or logic of the code. It doesn't provide a suggestion for code improvement or highlight a potential issue with the code itself.
14. test/fixtures/foo-bar-resolver-v2.js:26
- Draft comment:
Typographical consistency issue: the error message here refers toparserOptions.project, but on line 21 it useslanguageOptions.parserOptions.project. Consider updating this message to consistently referencelanguageOptions.parserOptions.project. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The inconsistency is real and in newly added code. The suggestion would make the error messages more consistent and clearer. However, both messages are technically understandable, and this is a very minor issue in test code. The comment is about style rather than functionality. The build would still pass with either message. The inconsistency might be intentional - perhaps the different paths are referenced differently in different contexts. Also, this is test code where perfect consistency might be less important. While test code quality matters less, consistent error messages make debugging easier. The inconsistency appears unintentional and easy to fix. The comment should be deleted. While it points out a real inconsistency, it's a minor stylistic issue in test code that doesn't affect functionality.
15. test/fixtures/foo-bar-resolver-v3.js:31
- Draft comment:
Typo/Inconsistency: The error messages fortsconfigdiffer in terminology. Line 26 uses "languageOptions.parserOptions.project" while line 31 uses only "parserOptions.project". Consider using the same reference (e.g., "languageOptions.parserOptions.project") 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% The inconsistency is real and in the changed code. However, the messages are still clear and unambiguous - they're just using different levels of specificity. The longer form is more precise but both forms effectively communicate the same thing. This seems like a very minor stylistic issue that doesn't impact functionality or maintainability significantly. The inconsistency could potentially cause confusion for future developers trying to debug issues. Consistent terminology in error messages is a good practice. While consistency is good, this difference is minor and both messages are clear enough. The comment feels too nitpicky for the small benefit it would provide. The comment should be deleted as it points out a very minor stylistic inconsistency that doesn't significantly impact code quality or clarity.
16. test/utils/resolve.spec.ts:772
- Draft comment:
Typo: The test description currently reads "respect project setting". Consider correcting it to "respects project setting" for grammatical accuracy. - 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% While technically correct, this is a very minor grammatical issue in a test description. Test descriptions are internal documentation and small grammatical issues don't impact functionality. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category. The grammatical correction is technically accurate. Poor grammar in tests could make them slightly harder to understand. While technically correct, the current grammar doesn't meaningfully impact test readability or maintainability. This is too minor to warrant a PR comment. Delete this comment as it's too minor and doesn't meaningfully impact code quality or maintainability.
Workflow ID: wflow_KWX4LJEiTUQdcu2z
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
test/fixtures/foo-bar-resolver-v2.js (2)
17-17: Fix grammatical error in assertion message.The error message contains a grammatical issue - "presented" should be "present".
- assert.ok(options.context.cwd, 'the `context.cwd` must be presented') + assert.ok(options.context.cwd, 'the `context.cwd` must be present')
18-28: Add test coverage for the conditional tsconfig validation branches.The conditional logic for validating
tsconfigpresence based onparserOptions.projectis not fully covered by tests. While the new test inresolve.spec.tscovers the case whereprojectistrue, there's no test covering theelsebranch whereprojectis falsy andtsconfigmust not be present.Please add test cases to cover both branches of the conditional validation:
- When
parserOptions.projectis set totrue, verify that a validtsconfigis provided- When
parserOptions.projectisfalse/undefined, verify thattsconfigis not provided#!/bin/bash # Check if there are any tests that exercise the else branch of the tsconfig conditional rg -A 10 -B 5 "project.*false\|project.*undefined\|project:\s*false" test/ --type js🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-19: test/fixtures/foo-bar-resolver-v2.js#L19
Added line #L19 was not covered by teststest/fixtures/foo-bar-resolver-no-version.js (1)
15-25: Add test coverage for conditional tsconfig validation logic.The conditional assertions for
tsconfigpresence based onparserOptions.projectare not covered by tests. This is the same issue identified in the other resolver fixtures.The current test in
resolve.spec.tsonly covers the case whereproject: true, but doesn't test theelsebranch whereprojectis falsy andtsconfigmust not be present. Please add tests to exercise both conditional paths.#!/bin/bash # Check test coverage for this specific fixture's conditional logic rg -A 5 -B 5 "foo-bar-resolver-no-version.*project\|project.*foo-bar-resolver-no-version" test/ --type js🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-16: test/fixtures/foo-bar-resolver-no-version.js#L16
Added line #L16 was not covered by teststest/fixtures/foo-bar-resolver-v1.js (1)
15-25: Missing test coverage for conditional tsconfig validation logic.The conditional assertions that validate
tsconfigpresence based onparserOptions.projectare not covered by existing tests. This is consistent with the same gap identified in the other resolver fixtures.While the new test "respect project setting" in
resolve.spec.tscovers the case whereproject: true, it doesn't test the scenario whereprojectis falsy andtsconfigmust not be present. Please add test cases covering both branches of the conditional logic to ensure complete validation.#!/bin/bash # Verify if any tests specifically target this fixture's conditional branches rg -A 5 -B 5 "foo-bar-resolver-v1" test/ --type js | rg -A 5 -B 5 "project.*false\|tsconfig.*false"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-16: test/fixtures/foo-bar-resolver-v1.js#L16
Added line #L16 was not covered by teststest/fixtures/foo-bar-resolver-v3.js (1)
23-33: Test coverage for conditional tsconfig validation is still incomplete.As noted in previous reviews, the conditional validation logic for
tsconfigbased onprojectsettings lacks comprehensive test coverage. The static analysis confirms that line 24 (and the surrounding conditional branches) remain untested.The conditional logic itself is correct - it enforces the contract that:
- When
parserOptions.projectis set →options.tsconfigmust be present- When
parserOptions.projectis not set →options.tsconfigmust be absentHowever, tests are needed to verify both branches execute correctly and that the assertions fire as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-24: test/fixtures/foo-bar-resolver-v3.js#L24
Added line #L24 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/ci.yml(1 hunks)src/utils/export-map.ts(6 hunks)src/utils/resolve.ts(8 hunks)test/fixtures/bar.tsx(1 hunks)test/fixtures/foo-bar-resolver-no-version.js(1 hunks)test/fixtures/foo-bar-resolver-v1.js(1 hunks)test/fixtures/foo-bar-resolver-v2.js(1 hunks)test/fixtures/foo-bar-resolver-v3.js(1 hunks)test/utils/resolve.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/fixtures/bar.tsx
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/resolve.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/utils/resolve.spec.ts (3)
src/utils/resolve.ts (2)
resolve(250-283)resolve(299-309)test/utils.ts (2)
testContext(155-161)testFilePath(39-41)src/types.ts (1)
NewResolver(73-78)
src/utils/export-map.ts (3)
src/types.ts (2)
ChildContext(147-157)RuleContext(140-145)src/utils/lazy-value.ts (2)
lazy(6-18)defineLazyProperty(22-52)src/utils/resolve.ts (3)
resolve(250-283)resolve(299-309)relative(230-238)
test/fixtures/foo-bar-resolver-v1.js (3)
test/fixtures/foo-bar-resolver-no-version.js (1)
project(6-6)test/fixtures/foo-bar-resolver-v2.js (1)
project(6-6)test/fixtures/foo-bar-resolver-v3.js (1)
project(12-12)
test/fixtures/foo-bar-resolver-v2.js (4)
.eslint-doc-generatorrc.js (1)
config(6-9)test/fixtures/foo-bar-resolver-no-version.js (1)
project(6-6)test/fixtures/foo-bar-resolver-v1.js (1)
project(6-6)test/fixtures/foo-bar-resolver-v3.js (1)
project(12-12)
🪛 GitHub Check: codecov/patch
src/utils/export-map.ts
[warning] 106-107: src/utils/export-map.ts#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 109-110: src/utils/export-map.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 113-113: src/utils/export-map.ts#L113
Added line #L113 was not covered by tests
test/fixtures/foo-bar-resolver-no-version.js
[warning] 16-16: test/fixtures/foo-bar-resolver-no-version.js#L16
Added line #L16 was not covered by tests
test/fixtures/foo-bar-resolver-v1.js
[warning] 16-16: test/fixtures/foo-bar-resolver-v1.js#L16
Added line #L16 was not covered by tests
test/fixtures/foo-bar-resolver-v2.js
[warning] 19-19: test/fixtures/foo-bar-resolver-v2.js#L19
Added line #L19 was not covered by tests
test/fixtures/foo-bar-resolver-v3.js
[warning] 24-24: test/fixtures/foo-bar-resolver-v3.js#L24
Added line #L24 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (160)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
🔇 Additional comments (6)
test/fixtures/foo-bar-resolver-v3.js (2)
3-3: LGTM: Assert import added appropriately.The addition of
assert/strictimport is necessary for the new validation logic and follows Node.js best practices.
10-22: LGTM: Options parameter integration is well-implemented.The resolver correctly accepts the new
optionsparameter and extracts the necessary context information. The conditional path logic based onprojectsettings aligns with the PR objectives of enhancing resolver capabilities while maintaining compatibility.src/utils/export-map.ts (4)
196-202: LGTM: Lazy tsconfig integration in ExportMap.get.The integration of lazy tsconfig evaluation using
defineLazyPropertyfollows the established pattern and ensures the tsconfig is only computed when needed by the resolver.
213-217: LGTM: Consistent tsconfig handling in ExportMap.parse.The lazy tsconfig setup and its usage for
isEsModuleInteropTruemaintains consistency with the overall tsconfig integration strategy.
289-295: LGTM: RemotePath function properly enhanced.The
remotePathfunction correctly passes the lazy tsconfig property to therelativefunction, maintaining the lazy evaluation pattern throughout the resolution chain.
1151-1167: LGTM: ChildContext function properly enhanced with required properties.The explicit inclusion of
cwd,filename, andphysicalFilenameensures these properties are always available in child contexts, which is essential for the new resolver options contract. This addresses the requirement forcontext.cwdthat is asserted in the resolver fixtures.
JounQin
force-pushed
the
feat/resolver_options
branch
from
f8e571b to
b18bca2
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed b18bca2 in 1 minute and 56 seconds. Click for details.
- Reviewed
182lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft 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:93
- Draft comment:
Exporting getTsconfigWithContext now makes it part of the public API. Confirm that exposing this function is intentional and that it won’t inadvertently break any internal encapsulation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This seems like a speculative comment asking the author to confirm their intention. The function appears to be used internally by the module's own code. The comment is not pointing out a clear issue, just raising a hypothetical concern. It's asking the author to verify their intention, which violates our rules. Maybe exposing internal configuration parsing logic could actually cause real problems for maintainability or create unwanted coupling with external code. While that's a valid concern, the comment doesn't demonstrate any actual problems - it's just asking for confirmation. If there were specific issues with exposing this function, they should be stated directly. This comment should be deleted because it's speculative and asks the author to confirm their intention, rather than pointing out a concrete issue.
2. src/utils/export-map.ts:102
- Draft comment:
The tsconfigRootDir assignment now uses a multi‐line construct with a commented fallback to context.cwd. Ensure the TODO is tracked and that consumers are informed about the upcoming change in fallback behavior. - 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 a TODO is tracked and that consumers are informed about a change. This falls under the category of asking the author to ensure something is done, which is against the rules.
3. test/utils/export-map.spec.ts:120
- Draft comment:
The createContext helper centralizes test context creation. Verify that it accurately mimics the full RuleContext structure required by getTsconfigWithContext and other functions. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. test/utils/export-map.spec.ts:540
- Draft comment:
Using jest.unstable_mockModule to mock 'get-tsconfig' is creative for isolating getTsconfigWithContext behavior. Confirm that this experimental API is stable enough for your test suite. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm the stability of usingjest.unstable_mockModule, which is not a specific code suggestion or a request for a test. It falls under the category of asking the author to ensure behavior, which is against the rules.
5. test/utils/export-map.spec.ts:569
- Draft comment:
The skipped test for falling back to cwd indicates planned changes. Make sure to revisit and enable this test in the next major release when context.cwd is used. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and suggests revisiting the test in the future. It doesn't provide a specific code suggestion or ask for a test to be written now. It also doesn't point out a potential issue with the current code.
Workflow ID: wflow_6rYhsXp8mzcJeSKk
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
b18bca2 to
b7f02d7
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b7f02d7 in 1 minute and 29 seconds. Click for details.
- Reviewed
182lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft 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:93
- Draft comment:
Exporting getTsconfigWithContext makes it available externally; please ensure its JSDoc is updated to reflect its public API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the author to ensure that the JSDoc is updated, which is similar to asking them to ensure something is done. This violates the rule against asking the author to ensure something is done. However, it does point out that the function is now part of the public API, which could be useful information. Still, the main focus is on ensuring documentation, which is not allowed.
2. src/utils/export-map.ts:102
- Draft comment:
The fallback for tsconfigRootDir now uses process.cwd() with a commented out context.cwd. Confirm that this backward‐compatible behavior is intentional and remember to update it in the next major release. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment is asking the author to confirm their intention regarding the use ofprocess.cwd()as a fallback fortsconfigRootDir. It also suggests remembering to update this in the next major release. The first part of the comment is not allowed as it asks for confirmation of intention, but the second part is a useful reminder for future updates.
3. test/utils/export-map.spec.ts:568
- Draft comment:
The test for falling back to cwd when tsconfigRootDir is not provided is currently skipped; ensure to enable it once the fallback behavior is updated. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment doesn't add any new information - it just restates what's already clearly indicated by the "TODO" comment and test.skip(). The code itself is self-documenting with the TODO comment. The PR author already knows they need to enable this test in the next major version. Maybe the comment could help emphasize the importance of not forgetting about this TODO? Or provide context about why it needs to wait for next major? No, the TODO comment in code is sufficient tracking. If more context is needed, it should be added to the TODO comment itself, not as a PR comment. Delete the comment as it just restates what's already clear from the code's TODO comment and skipped test.
Workflow ID: wflow_PtAmyxq7nKrbiVbM
You can customize
JounQin
force-pushed
the
feat/resolver_options
branch
from
b7f02d7 to
ab2d06f
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ab2d06f in 1 minute and 6 seconds. Click for details.
- Reviewed
444lines of code in9files - Skipped
0files when reviewing. - Skipped posting
4draft 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. test/fixtures/foo-bar-resolver-no-version.js:9
- Draft comment:
Consider aligning the assert messages (e.g. for tsconfig presence) with those in resolver-v1 for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. test/utils/export-map.spec.ts:24
- Draft comment:
Avoid shadowing the built‐in 'path' module. Rename the local variable to improve clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. test/utils/resolve.spec.ts:487
- Draft comment:
Consider adding inline comments in the 'rename cache correctness' block to clarify expected cache invalidation behavior post-rename. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. test/utils/resolve.spec.ts:21
- Draft comment:
Great job—this test suite shows excellent coverage for various resolver interfaces and caching scenarios. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_WlHTToYCEW6tGz8j
You can customize
|
Test coverage is finally passing. |
related:
context.getCwd()available in resolver import-js/eslint-plugin-import#2108optionsargument for resolver import-js/eslint-plugin-import#2519Important
Introduces a new
optionsargument for resolvers, enhancing configurability while maintaining compatibility, with updated documentation and tests.optionsargument to resolver functions for enhanced configuration, maintaining backward compatibility.export-map.ts.types.tsto acceptoptions.resolvers/README.mdto includeoptionsargument details.cwdis consistently available in resolver context.resolve.spec.tsandexport-map.spec.tsfor new resolver behavior and TypeScript configuration caching.This description was created by
for ab2d06f. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
optionsargument in resolver functionality, enabling enhanced configuration while maintaining backward compatibility.Documentation
optionsargument and its structure, with detailed examples and references.Bug Fixes
cwd) is consistently available during resolution.Tests