Skip to content

[Fix] add missing optional peer dependencies #2283

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: main
Choose a base branch
from
Open

[Fix] add missing optional peer dependencies #2283

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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ rules:

You may use the following shortcut or assemble your own config using the granular settings described below.

Make sure you have installed [`@typescript-eslint/parser`] which is used in the following configuration. Unfortunately NPM does not allow to list optional peer dependencies.
Make sure you have installed [`@typescript-eslint/parser`] which is used in the following configuration.

```yaml
extends:
Expand Down
5 changes: 5 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@
"typescript": "^2.8.1 || ~3.9.5",
"typescript-eslint-parser": "^15 || ^22.0.0"
},
"peerDependenciesMeta": {
"@typescript-eslint/parser": {
"optional": true
}
},
"peerDependencies": {
"eslint": "^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0 || ^8"
},
Expand Down
1 change: 1 addition & 0 deletions utils/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## Unreleased
- add missing optional peer dependencies ([#2283], thanks [@jdanil])

## v2.7.1 - 2021-10-13

Expand Down
8 changes: 8 additions & 0 deletions utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
"url": "https://github.com/import-js/eslint-plugin-import/issues"
},
"homepage": "https://github.com/import-js/eslint-plugin-import#readme",
"peerDependenciesMeta": {
"@typescript-eslint/parser": {
"optional": true
},
"eslint-import-resolver-node": {
Copy link
Member

Choose a reason for hiding this comment

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

This isn’t needed, because it’s already available as a dependency, so the node resolution algorithm means it’s already accessible. There are some broken package managers that require this unnecessary peer dep declaration to make it accessible, but I’m not going to add a breaking change just to cater to broken package managers.

Copy link
Author

Choose a reason for hiding this comment

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

eslint-import-resolver-node is a dependency of eslint-plugin-import, but not of eslint-module-utils where it is required by default.

Copy link
Member

Choose a reason for hiding this comment

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

eslint-module-utils actually dynamically requires eslint-import-resolver-X, for every X defined in the user's eslint settings.

That the node resolver is the default doesn't mean someone has to use it, and that you can specify a thousand different resolvers (that eslint-module-utils will dutifully try to require) doesn't mean eslint-module-utils should be specifying it.

This kind of dynamic requiring is simply a normal part of the way the entire eslint ecosystem works, and any package manager that breaks it is itself broken.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, and if a consumer is using a non-default resolver, it is up to them to declare it as a dependency. eslint-module-utils is hard coded to use eslint-import-resolver-node by default, which is why it makes sense for it to be declared as an optional peer dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, this is the right behavior and this causes any projects using yarn v3 to fail without custom configuration. Perhaps the yarn documentation will make it more clearn as-to why: https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - that's what require.resolve is for :)

I think we need an example that breaks. I'm in Camp Pure Declarations but plugins cannot be known beforehand so cannot be declared, and AFAIK things just work right now @LuckyMona

Copy link

@LuckyMona LuckyMona Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks very much for all your masters' replies, but I think maybe the follow things also need your attention:

it's simply not possible for this package to explicitly list every plugin package that a user might want to instruct it to load.

Sorry, I'm a little not agree with it. Let me explain my findings when I try to debug this error:
Here is my eslint config:
image

When I debug, I found these two items all can be resolved right, but the varible of settings has an extra part, here is its runtime value:

{
  "<project path>/node_modules/.store/eslint-import-resolver-node-npm-0.3.6-d9426786c6/node_modules/eslint-import-resolver-node/index.js": {
  },
  "<project path>/node_modules/.store/eslint-import-resolver-typescript-virtual-ae492bc0db/node_modules/eslint-import-resolver-typescript/lib/cjs.js": {
  },
  node: {
    extensions: [
      ".mjs",
      ".js",
      ".json",
    ],
  },
}

and the error happens when it deals with the extra partnode:{extensions: [".mjs", ".js", ".json",],}, which I think might be added by your code in the opts function in eslint-plugin-import-main/resolvers/node/index.js, because I don't config it in my project.

So I think maybe you should add node resolver to the peer deps.

(Sorry for my foolish nonsense)

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the error you're getting? I don't understand how these settings might be problematic

Copy link
Collaborator

Choose a reason for hiding this comment

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

A reproduction is really required here. 😅

Copy link
Member

Choose a reason for hiding this comment

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

@LuckyMona eslint config must be able to be static - not JS, but JSON/ini. As such, require.resolve is not available in every case to the config.

"optional": true
}
},
"dependencies": {
"debug": "^3.2.7",
"find-up": "^2.1.0",
Expand Down