Skip to content

feat(no-extraneous-dependencies): allow package to import itself #309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+91 −10
Open

feat(no-extraneous-dependencies): allow package to import itself #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/cruel-cooks-notice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-import-x": patch
---

Allow packages to import themselves in `import-x/no-extraneous-imports` if the `exports` field is defined in `package.json`
65 changes: 55 additions & 10 deletions src/rules/no-extraneous-dependencies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ function readJSON<T>(jsonPath: string, throwException: boolean) {

function extractDepFields(pkg: PackageJson) {
return {
name: pkg.name,
exports: pkg.exports,
dependencies: pkg.dependencies || {},
devDependencies: pkg.devDependencies || {},
optionalDependencies: pkg.optionalDependencies || {},
Expand Down Expand Up @@ -69,6 +71,8 @@ function getDependencies(context: RuleContext, packageDir?: string | string[]) {

try {
let packageContent: PackageDeps = {
name: undefined,
exports: undefined,
dependencies: {},
devDependencies: {},
optionalDependencies: {},
Expand All @@ -91,10 +95,30 @@ function getDependencies(context: RuleContext, packageDir?: string | string[]) {
paths.length === 1,
)
if (packageContent_) {
for (const depsKey of Object.keys(packageContent)) {
const key = depsKey as keyof PackageDeps
Object.assign(packageContent[key], packageContent_[key])
if (!packageContent.name) {
packageContent.name = packageContent_.name
packageContent.exports = packageContent_.exports
}
Object.assign(
packageContent.dependencies,
packageContent_.dependencies,
)
Object.assign(
packageContent.devDependencies,
packageContent_.devDependencies,
)
Object.assign(
packageContent.optionalDependencies,
packageContent_.optionalDependencies,
)
Object.assign(
packageContent.peerDependencies,
packageContent_.peerDependencies,
)
Object.assign(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JounQin JounQin May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping @remcohaszing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by reverting to the previous style? This is not a stylistic change.

Copy link
Member

@JounQin JounQin May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't they? Only name and exports are special, all other keys can key previous style assignment.

Copy link
Member

@JounQin JounQin May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, should we check whether the importee is truly exported in exports? Example:

import 'fake-pkg/subpath'

/subpath needs to be checked whether available in exports?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all other keys can key previous style assignment.

I really don’t understand what style you envision. Please elaborate. Do you mean the loop? That doesn’t work. Not all keys are objects that should be shallow merged.

By the way, should we check whether the importee is truly exported in exports?

No. That doesn’t change anything. If fake-pkg uses its own package exports, it uses the exports from its own package.json. It no longer checks node_modules/fake-pkg/package.json. So even if the fake-pkg/subpath export doesn’t exist, it still doesn’t resolve to implicit dependencies.

Copy link
Member

@JounQin JounQin May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don’t understand what style you envision. Please elaborate. Do you mean the loop? That doesn’t work. Not all keys are objects that should be shallow merged.

Loop with check name and exports specially only, all other keys should not be changed.

So even if the fake-pkg/subpath export doesn’t exist, it still doesn’t resolve to implicit dependencies.

I'm not sure to understand, I know <rootDir>/node_modules/fake-pkg/package.json won't be checked, I'm talking about <rootDir>/package.json, and also, when the user installs fake-pkg manually or in its dependency tree implicitly, what means <rootDir>/node_modules/fake-pkg/package.json is present, what'll happen?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you're both right

@remcohaszing i think you may be better off having a set of known merged fields, like:

const fieldsToMerge = ['dependencies', 'devDependencies', /* ... */];
for (const field of fieldsToMerge) {
  // if the package manifest has it, merge it into `packageContent[field]`
}

you're right we probably shouldn't blindly merge every key now that not all keys are objects (and that may change in future if we add even more non-object keys).

i think @JounQin is just suggesting you keep the existing "deep merge every key", but you'd have to have a condition in there to skip name and exports. then it'd achieve the same as what you have here

but imo we should just be explicit about the fields we want to merge

packageContent.bundledDependencies,
packageContent_.bundledDependencies,
)
}
}
} else {
Expand All @@ -111,13 +135,17 @@ function getDependencies(context: RuleContext, packageDir?: string | string[]) {
}

if (
![
packageContent.dependencies,
packageContent.devDependencies,
packageContent.optionalDependencies,
packageContent.peerDependencies,
packageContent.bundledDependencies,
].some(hasKeys)
!(
packageContent.name ||
packageContent.exports ||
[
packageContent.dependencies,
packageContent.devDependencies,
packageContent.optionalDependencies,
packageContent.peerDependencies,
packageContent.bundledDependencies,
].some(hasKeys)
)
) {
return
}
Expand Down Expand Up @@ -297,6 +325,20 @@ function reportIfMissing(
return
}

if (importPackageName === deps.name) {
if (!deps.exports) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand why exports field is required, and is this prevents self-importing issues in source files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Hello

If 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.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JounQin JounQin Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to summary, import/require via package's own exports is supported by Node, main is not, right?

Correct!

Copy link
Member

@JounQin JounQin May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remcohaszing Would you like to combine your proposed import-x/no-own-exports rule into this PR together? Or maybe wait for other reviewers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That rule seems unrelated to these changes TBH. Also I don’t have personal interest in that rule.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that rule should be added together with this behavior change IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

context.report({
node,
messageId: 'selfImport',
data: {
packageName,
},
})
}

return
}

if (declarationStatus.isInDevDeps && !depsOptions.allowDevDeps) {
context.report({
node,
Expand Down Expand Up @@ -358,6 +400,7 @@ export type MessageId =
| 'devDep'
| 'optDep'
| 'missing'
| 'selfImport'

export default createRule<[Options?], MessageId>({
name: 'no-extraneous-dependencies',
Expand Down Expand Up @@ -392,6 +435,8 @@ export default createRule<[Options?], MessageId>({
"'{{packageName}}' should be listed in the project's dependencies, not optionalDependencies.",
missing:
"'{{packageName}}' should be listed in the project's dependencies. Run 'npm i -S {{packageName}}' to add it",
selfImport:
"'{{packageName}}' may only import itself if the exports field is defined in package.json",
},
},
defaultOptions: [],
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/package-named-exports/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default function () {}
1 change: 1 addition & 0 deletions test/fixtures/package-named-exports/node_modules/package-named-exports/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions test/fixtures/package-named-exports/node_modules/package-named-exports/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions test/fixtures/package-named-exports/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "package-named-exports",
"description": "Standard, named package with exports",
"exports": "./index.js"
}
1 change: 1 addition & 0 deletions test/fixtures/package-named/node_modules/package-named/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions test/fixtures/package-named/node_modules/package-named/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions test/rules/no-extraneous-dependencies.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,11 @@ ruleTester.run('no-extraneous-dependencies', rule, {
{ packageDir: packageDirMonoRepoRoot, whitelist: ['not-a-dependency'] },
],
}),
tValid({
code: 'import "package-named-exports"',
filename: testFilePath('package-named-exports/index.js'),
options: [{ packageDir: testFilePath('package-named-exports') }],
}),
],
invalid: [
tInvalid({
Expand Down Expand Up @@ -446,6 +451,14 @@ ruleTester.run('no-extraneous-dependencies', rule, {
{ messageId: 'missing', data: { packageName: 'not-a-dependency' } },
],
}),
tInvalid({
code: 'import "package-named"',
filename: testFilePath('package-named/index.js'),
options: [{ packageDir: testFilePath('package-named') }],
errors: [
{ messageId: 'selfImport', data: { packageName: 'package-named' } },
],
}),
],
})

Expand Down