fix(types): incorrect and unfinished NodeResolverOptions typing
#15
Conversation
🦋 Changeset detectedLatest commit: dd3ac5c 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 |
WalkthroughThis update expands and corrects the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Module
participant Resolver
User->>Module: Import/require with options
Module->>Resolver: Passes NodeResolverOptions (with new structure)
Resolver-->>Module: Resolves module using updated options
Module-->>User: Returns resolved module
Possibly related PRs
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/types.tsOops! Something went wrong! :( ESLint: 9.27.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (7)
✨ 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR corrects and completes the NodeResolverOptions type definition and adjusts a legacy resolver signature, plus pins a local link for ESLint context.
- Extends
NodeResolverOptionsfromNapiResolveOptions, reintroducing and deprecating fields with accurate JSDoc - Changes
LegacyResolverResolveImportreturn type fromstring | undefinedtostring - Adds a Yarn resolution linking
eslint-import-contextto the local package
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/types.ts | Expanded NodeResolverOptions typing and updated resolver return |
| package.json | Added Yarn resolution for eslint-import-context |
Comments suppressed due to low confidence (2)
src/types.ts:102
- The return type for
LegacyResolverResolveImportwas changed fromstring | undefinedtostring, which could break callers expectingundefined. Consider restoring| undefinedor updating upstream code to handle only string results.
) => string
package.json:103
- [nitpick] Using a
link:.resolution foreslint-import-contextis non-standard and may confuse collaborators. Consider documenting this override in README or using a fixed version instead.
"eslint-import-context": "link:."
|
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. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
commit: |
|
Deploy preview for eslint-import-context ready! ✅ Preview Built with commit dd3ac5c. |
📊 Package size report 2%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 53 53
Branches 16 16
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 1bed527 in 1 minute and 39 seconds. Click for details.
- Reviewed
74lines of code in2files - Skipped
1files 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. package.json:102
- Draft comment:
Verify that local linking via 'link:.' in resolutions is intentional and won't conflict with dependency resolution. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to verify their intention regarding a specific change in dependency resolution. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific suggestion or point out a clear issue.
2. src/types.ts:40
- Draft comment:
Review union types (string | readonly string[]) in NodeResolverOptions; ensure this flexibility and the extension via Omit<NapiResolveOptions, 'extensions'> are intended. - 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 asking the author to verify their intentions rather than pointing out a specific issue. The union types are well-documented with clear purposes (e.g. "Array of file extensions" for extensions, "Directory (or directories)" for moduleDirectory). The Omit usage seems intentional to override the extensions type from the base interface. The types could potentially cause runtime issues if not handled properly in the implementation. The comment might be trying to prevent potential bugs. The types are clearly documented and appear intentional. If there were implementation issues, they would be caught during testing or type checking. Delete the comment as it merely asks for confirmation of intentional changes rather than pointing out a specific issue that needs fixing.
3. src/types.ts:65
- Draft comment:
LegacyResolverResolveImport return type changed from 'string | undefined' to 'string'; ensure that legacy implementations correctly handle cases where undefined was expected. - 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% This is a breaking change in the type definition that removes the possibility of returning undefined. This could cause runtime errors if existing implementations rely on undefined returns. However, the comment is asking for verification rather than pointing out a specific issue. It starts with "ensure that..." which violates our rules. We should not ask authors to verify things. The type change could be intentional and well-tested. Without seeing the implementations, we can't be certain this is actually a problem. While the type change is significant, asking for verification doesn't add value. If there were actual problems, they would show up in tests or TypeScript errors. Delete the comment because it asks for verification rather than pointing out a specific issue. If there are problems, they will be caught by TypeScript compilation.
Workflow ID: wflow_AWVO4epGYivMT9K9
You can customize 
size-limit report 📦
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed dd3ac5c in 1 minute and 43 seconds. Click for details.
- Reviewed
86lines of code in3files - Skipped
1files 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. .changeset/swift-carrots-heal.md:1
- Draft comment:
Changeset file looks valid. Ensure YAML quotes are intentional. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. package.json:102
- Draft comment:
Local linking for 'eslint-import-context' is set in resolutions. Confirm this aligns with dependency usage. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
3. src/types.ts:41
- Draft comment:
Expanded NodeResolverOptions improves flexibility. Update the 'extensions' comment to mention that a single string is allowed as shorthand for an array. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/types.ts:163
- Draft comment:
Allowing webpack options as boolean is a sensible addition. Ensure downstream consumers correctly handle a boolean value. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/types.ts:56
- Draft comment:
In the comment for themoduleDirectoryoption, the phrase "(default to 'node_modules')" appears. Consider changing it to "(defaults to 'node_modules')" for improved grammatical 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% This is a very minor grammatical suggestion. The code is actually copied from DefinitelyTyped types as noted in the comment above. Making small grammatical changes to copied documentation could cause confusion or divergence from the source. The current wording is perfectly understandable even if not optimal. The grammatical issue does make the documentation slightly less professional. Consistency in documentation is important for maintainability. While documentation consistency is valuable, this is an extremely minor issue that doesn't impact code functionality or understanding. The comment is about copied documentation where maintaining alignment with the source is more important. Delete this comment as it suggests an extremely minor grammatical change to documentation that was intentionally copied from another source.
Workflow ID: wflow_ybHiFIXHsyl6TqNg
You can customize
Important
Fixes and completes
NodeResolverOptionstyping intypes.ts, and updateseslint-import-contextresolution inpackage.json.NodeResolverOptionstyping intypes.ts, adding properties likebasedir,includeCoreModules,extensions,paths,moduleDirectory, andpreserveSymlinks.package,packageFilter,pathFilter, andpackageIteratoras deprecated inNodeResolverOptions.eslint-import-contextresolution inpackage.json.This description was created by
for dd3ac5c. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Chores