[Fix] add missing optional peer dependencies

[jdanil]

The plugin uses @typescript-eslint/parser as the default parser for typescript files in the exported typescript config. eslint-import-resolver-node is also used as the default resolver.

This PR adds the packages as optional peer dependencies so that they can be safely accessed.

[codecov]

Codecov Report

Merging #2283 (96e48ca) into main (6682e9a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2283   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files          65       65           
  Lines        2687     2687           
  Branches      889      889           
=======================================
  Hits         2543     2543           
  Misses        144      144           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6682e9a...96e48ca. Read the comment docs.

[ljharb]

This is a breaking change for npm versions that lack optional peer dep support.

[ljharb]

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.

[jdanil]

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

[ljharb]

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.

[jdanil]

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.

[bradennapier]

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

[wmertens]

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

[LuckyMona]

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:

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)

[wmertens]

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

[JounQin]

A reproduction is really required here. 😅

[ljharb]

@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.

[jdanil]

This is a breaking change for npm versions that lack optional peer dep support.

Hi @ljharb. I'm not sure I understand why this would be a breaking npm <7. If the package manager doesn't support optional peer dependencies, then wouldn't the behaviour be the same as it is now?

[ljharb]

Peer dependencies are always required, unless the package manager supports optional peer deps and it's also declared as such.

So, since adding a non-optional peer dependency is a breaking change, this would be a breaking change as well.

[jdanil]

This change doesn't add the dependencies to peerDependencies though, so there shouldn't be any impact there. They're only added to peerDependenciesMeta as optional. So packages that do not support peerDependenciesMeta will have no impact.

[ljharb]

ah, i see what you mean.

That confuses me tho. peerDependenciesMeta is only intended to describe things listed in peerDependencies. It's not supposed to have any effect to put something in there that's not mentioned in peer deps.

Is this something you're observing in npm, or in an alternative client?

[jdanil]

If marking a dependency as optional in peerDependenciesMeta it implicitly considers that to be "dependency": "*" if no specific version ranges have been listed in peerDependencies.

So it seems like that would allow us to avoid any breaking changes as there is no need to modify peerDependencies.

I'm using yarn myself.

[ljharb]

Presumably you're using PnP? normal yarn matches npm afaik, which doesn't require anything to be marked as a peer dep to be "accessible".

[jdanil]

Yep I am running PnP in CI for now and node modules linker locally. The node modules linker in both yarn and npm seem to work in this case as they are accessing these transitive dependencies. But since they are being explicitly required in these packages it would seem to be correct for them to be declared as peer dependencies.

[ljharb]

I believe that's because yarn itself has hardcoded these examples for this plugin.

It's not "more correct" because there's nothing correct about requiring a declaration. The correct thing is for transitive dependencies to be nondeterministically available to the application.

[teohhanhui]

Accessing transitive dependencies is inherently unsafe, regardless of package manager. Things might just break when your dependencies change their dependencies...

[ljharb]

In the case of the TS parser, you'd have it as an explicit top-level dep if it's used in your config (or a shared config would have it as a peer dep). In the case of the node resolver, it's a runtime dep of this plugin, so we can be certain we'll never remove it.

[jdanil]

eslint-plugin-import isn't the only consumer of eslint-module-utils though. There are other plugins like https://github.com/azz/eslint-plugin-monorepo which use the utils to resolve paths. If you're saying that these packages too should add eslint-import-resolver-node as a dependency if they intend to use the default resolver then that seems like exactly what peer dependencies are designed to indicate.

Even if we're just considering the node modules linker, without having these peer dependencies explicitly declared, strict node modules layouts like what pnpm uses would not necessarily have access to these packages.

In your view, what is the purpose of optional peer dependencies is if not for this?

[ljharb]

For one, anything npm doesn't do isn't something I think needs to be catered to - npm is the standard. That said, npm will be adding a pnpm-like "isolated mode" soon, at which point I'd be obliged to address it somehow.

See #2283 (comment) for why eslint-module-utils is correct as-is - it, like eslint itself, dynamically requires things based on config, and thus, it's always up to consumers to ensure that the things their config references are installed.

[jdanil]

npm may have been grandfathered into node, but I wouldn't say it is the "standard". Node now provides yarn and pnpm out of the box via corepack.

I still don't understand what you think the purpose of optional peer dependencies are if you think that all packages should have access to all transitive dependencies.

You obviously have some very strong opinions about what package manager users of eslint-plugin-import should use. I'm not going to argue. I just thought I'd try to contribute a fix to resolve the issue of undeclared dependencies, which it sounds like you will eventually have to address once it becomes an issue with npm.

[ljharb]

The sole purpose of optional peer deps is for something that, if present, must conform to a version range, but need not be present.

[wmertens]

The sole purpose of optional peer deps is for something that, if present, must conform to a version range, but need not be present.

I would argue that the purpose of optional peer deps is to notify the package manager that you would like access to these peer deps if they are in the project. If there is official documentation stating otherwise I'd love to find out.

I'm writing a package manager of sorts for Nix, where it would be most efficient if each package would declare their (optional) (peer) dependencies correctly, so that those can be symlinked together instead of copied.

I can confirm that pnpm treats peerDependenciesMeta packages that are not mentioned elsewhere as peerDependencies matching any version. Npm and Yarn copy things together so they leak access that way.

[wmertens]

Incidentally, utils also requires eslint to find espree, which means eslint or espree are peer dependencies. Since every project that uses this plugin also uses eslint, I'd argue that eslint should be a peer dependency.

[ljharb]

That’s not what it means - utils looks for espree/eslint inside a try/catch, which isn’t a peer relationship. Peer deps aren’t for defining “allowed dependencies to access”, optional or otherwise.