feat(no-extraneous-dependencies): allow package to import itself #309
Conversation
Allow packages to import themselves via the `package.json` exports field. If a package tries this, but the exports field is not defined, a new message is reported. Closes un-ts#305
🦋 Changeset detectedLatest commit: 4e0dc4f 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 change updates the Changes
Sequence Diagram(s)sequenceDiagram
participant ESLintRule as no-extraneous-dependencies
participant PackageJson as package.json
participant ImportStatement as Import Statement
ImportStatement->>ESLintRule: import { foo } from "package-name"
ESLintRule->>PackageJson: Read name and exports fields
alt Package name matches import & exports field exists
ESLintRule-->>ImportStatement: Allow import (no error)
else Package name matches import & no exports field
ESLintRule-->>ImportStatement: Report selfImport error
else Import is not self-import
ESLintRule-->>ImportStatement: Perform standard dependency check
end
Assessment against linked issues
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/rules/no-extraneous-dependencies.tsOops! Something went wrong! :( ESLint: 9.24.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/rules/no-extraneous-dependencies.spec.tsOops! Something went wrong! :( ESLint: 9.24.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/fixtures/package-named-exports/index.jsOops! Something went wrong! :( ESLint: 9.24.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (5)
🧰 Additional context used🧬 Code Graph Analysis (1)test/rules/no-extraneous-dependencies.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (21)
🔇 Additional comments (12)
✨ 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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 4e0dc4f in 1 minute and 59 seconds
More details
- Looked at
215lines of code in9files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. src/rules/no-extraneous-dependencies.ts:71
- Draft comment:
Initializing packageContent with 'name' and 'exports' properties set to undefined is clear. Consider adding a comment explaining that these fields are merged only once from the first package.json to avoid later overwrites. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. src/rules/no-extraneous-dependencies.ts:328
- Draft comment:
Self-import block: verifies if a package imports itself and reports an error if the 'exports' field is missing. The implementation meets the issue requirements. A short inline comment might help clarify the intention. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. .changeset/cruel-cooks-notice.md:5
- Draft comment:
The changeset notice refers toimport-x/no-extraneous-importsbut the actual rule is namedno-extraneous-dependencies. Please update the notice 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 = 10% vs. threshold = 50%
Without seeing the actual ESLint plugin code or documentation, I cannot verify if the rule is actually named 'no-extraneous-dependencies'. The changeset is introducing a new feature for a rule named 'import-x/no-extraneous-imports', and we should assume the author knows the correct name of their own rule. The comment makes an assertion without providing evidence.
I could be wrong if there's established naming conventions for ESLint rules that I'm not aware of, or if there's documentation elsewhere showing the correct rule name.
Even if there are conventions, we need clear evidence that the current name is wrong. Without such evidence, we should trust the author's knowledge of their own codebase.
Delete this comment as it makes an assertion about the rule name being wrong without providing any evidence to support the claim.
4. src/rules/no-extraneous-dependencies.ts:44
- Draft comment:
Good job addingnameandexportsto the extracted package fields. This extension is key for handling self-imports. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. src/rules/no-extraneous-dependencies.ts:97
- Draft comment:
Merging the package’s name and exports from the first package.json is reasonable, but note that when multiple packageDirs are provided, only the first one’s name/exports is used. Ensure this merging behavior is as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that the merging behavior is intended, which violates the rule against asking for confirmation of intention. It does not provide a specific suggestion or point out a clear issue with the code.
6. src/rules/no-extraneous-dependencies.ts:329
- Draft comment:
The new self-import check is clear: if the imported package name matches the package’s own name and the exports field is missing, a 'selfImport' error is reported. This meets the requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. test/rules/no-extraneous-dependencies.spec.ts:317
- Draft comment:
There's a potential typo in the filename 'foo.tes.js' used in the tInvalid test cases (e.g., at line 317). Did you mean 'foo.test.js'? Please verify if this was intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_y4n7hpxvA0T9NPK8
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| packageContent.peerDependencies, | ||
| packageContent_.peerDependencies, | ||
| ) | ||
| Object.assign( |
There was a problem hiding this comment.
Merging bundledDependencies using Object.assign may not properly concatenate arrays if multiple package.json files provide them. Consider whether concatenating arrays (e.g. using array spread or concat) might be more appropriate here.
There was a problem hiding this comment.
Personally I don't like current refactor, it's too verbose to me, only name and exports are special.
@remcohaszing Could you revert it back to previous style? So that I can merge this PR as-is.
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #309 +/- ##
==========================================
+ Coverage 95.99% 96.00% +0.01%
==========================================
Files 91 91
Lines 4744 4756 +12
Branches 1785 1791 +6
==========================================
+ Hits 4554 4566 +12
Misses 189 189
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -297,6 +325,20 @@ function reportIfMissing( | |||
| return | |||
| } | |||
|
|
|||
| if (importPackageName === deps.name) { | |||
| if (!deps.exports) { | |||
There was a problem hiding this comment.
I'm not sure to understand why exports field is required, and is this prevents self-importing issues in source files?
There was a problem hiding this comment.
I don’t know the rationale behind that decision, but that is how Node.js behaves. You can try it by creating a directory with two files:
package.json:
{
"name": "pkg",
"exports": "./index.js"
}index.js:
require('pkg')
console.log('Hello')If you run index.js, this logs:
$ node index.js
HelloIf you now remove the exports field, or change it to main, this logs:
$ node index.js
node:internal/modules/cjs/loader:1228
throw err;
^
Error: Cannot find module 'pkg'
Require stack:
- /home/remco/Downloads/asd/index.js
at Function._resolveFilename (node:internal/modules/cjs/loader:1225:15)
at Function._load (node:internal/modules/cjs/loader:1055:27)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:220:24)
at Module.require (node:internal/modules/cjs/loader:1311:12)
at require (node:internal/modules/helpers:136:16)
at Object.<anonymous> (/home/remco/Downloads/asd/index.js:1:1)
at Module._compile (node:internal/modules/cjs/loader:1554:14)
at Object..js (node:internal/modules/cjs/loader:1706:10)
at Module.load (node:internal/modules/cjs/loader:1289:32) {
code: 'MODULE_NOT_FOUND',
requireStack: [ '/home/remco/Downloads/asd/index.js' ]
}
Node.js v22.14.0There was a problem hiding this comment.
Even if it works for self-importing, but I personally still believe it's not a good practice to do like this as circular dependency itself. It should only be allowed in non-source files IMO.
There was a problem hiding this comment.
This example is indeed circular, which is a bit silly. This was to keep it minimal. Modules can resolve package-local modules using package.json exports. Tests are one useful example, but I can think of other use cases.
The goal of this PR however is not to discuss whether or not people should do this or when. People can do it. The concept of what a source file is also varies per project. The goal of this PR is to fix a false positives for detecting imports of extraneous modules.
There was a problem hiding this comment.
People can do anything they want without enabling this rule, the ESLint rules are for good practice, not how codes can be used in runtime. Otherwise, running the codes itself already helps you confirming it's working.
But I'd like to hear more voices from @thepassle @SukkaW @Shinigami92 @43081j
There was a problem hiding this comment.
So, to summary,
import/requirevia package's ownexportsis supported by Node,mainis not, right?
Correct!
There was a problem hiding this comment.
@remcohaszing Would you like to combine your proposed import-x/no-own-exports rule into this PR together? Or maybe wait for other reviewers.
There was a problem hiding this comment.
That rule seems unrelated to these changes TBH. Also I don’t have personal interest in that rule.
There was a problem hiding this comment.
Well, that rule should be added together with this behavior change IMO.
There was a problem hiding this comment.
That makes sense to me
We can probably merge this one first @JounQin and one of us can work on the new rule separately.
It looks like this won't change existing behaviour other than making this edge case work. So should be an easy one to land
Allow packages to import themselves via the
package.jsonexports field.If a package tries this, but the exports field is not defined, a new message is reported.
Closes #305
Important
Allow self-imports in
no-extraneous-dependenciesifexportsfield is defined inpackage.json, with error reporting for missingexports.no-extraneous-dependenciesifexportsfield is defined inpackage.json.selfImporterror if a package imports itself withoutexportsfield.getDependencies()inno-extraneous-dependencies.tsto includenameandexports.reportIfMissing()to handle self-import logic.exportsinno-extraneous-dependencies.spec.ts.exportsinno-extraneous-dependencies.spec.ts.This description was created by
for 4e0dc4f. It will automatically update as commits are pushed.
Summary by CodeRabbit