eslint-module-utils: add peer dependency on eslint #2523
Conversation
This allows it to find eslint from module-require.js when the package manager is strict. Maintainer note: this should not be necessary, but using `peerDependenciesMeta` here seems to have no downsides for real npm users, and allows tools like pnpm or yarn PnP to statically "know" that this package wants to access `eslint` if it is present.
|
Duplicate of #2283 |
|
Not quite a duplicate - that PR uses optional deps and does not include |
|
I agree it’s not a duplicate, but i don’t think we should be adding an unnecessary peer dep requirement due to behavior that npm doesn’t have. |
|
@wmertens Or can you point out what issue are you facing without this change? A reproduction or screenshot would be appreciated. |
|
I'm making a pure, lazy builder that can generate node_modules directories on the fly as a tree of symlinks to read-only packages, based on Nix (see nix-community/dream2nix#195). This allows full reuse of packages across projects and also instant updates when you check out a different commit with a different package-lock.json file. The result looks like this, very pure and fast: If I have 2 projects with the same dependencies, the builder will give them the exact same node_modules symlink. Node.js allows symlinks to modules, but will resolve packages based on the actual location of a package. So if a package doesn't have a dependency in its own node_modules or a parent directory, it won't find it. This is working, but some (very few) packages don't specify all their dependencies and they don't work. For eslint-import-utils, this means that it can't find espree. By adding this peer dependency, package managers know that eslint-import-utils should have access to eslint and thus espree. Since all users of the plugin will import eslint, this is the simplest peer dep to fix the build. |
|
That's using peer deps as a hack. If your module resolution algorithm isn't able to replicate what npm does by default, then it's broken (this applies to pnpm and yarn PnP as well). |
|
@ljharb I understand your position, but it's akin to saying "You shouldn't ban smoking, we've been smoking for centuries" in the 80s, no? On one hand, your setup obviously works with npm, but on the other hand, there are nice optimizations possible if you add this harmless short string to your package.json. |
|
No, it's saying you shouldn't allow smoking because it covers up the smell of something else :-) |
|
:-) How about a security perspective? Tools should only get access to what they request, and |
|
That's not a security property that node applications have, and I'm unconvinced it's one they should have. Literally anything that has static config (including eslint config) needs to be able to dynamically access packages even if they're not declared explicitly at the top level. Every package is supposed to have access to every other package. There's no security vulnerability here, just a perceived ideological flaw, and adding complexity to both tools and many packages in the ecosystem is a high cost for a purely ideological concern. |
|
Hmm. Indeed "perceived ideological flaw" is a good way to put it. But this ideology does result in simpler and more efficient build tools, at the tiny cost of slightly more metadata. My eslint config works perfectly fine because I use How about this:
|
|
I said static eslint config (JSON, primarily), which is still supported and something that - for maintainability reasons - is much more reliable than dynamic config. If the first bullet point were true, I'd be happy to merge it. However, declaring a peer dep forces downstream users to have eslint installed, which can cause it to be automatically installed by tools, which can prevent them from realizing they need to depend on an explicit version range of eslint in their own package.json. The only tools that would have to work around it are ones trying to use an incorrect node_modules layout. Sacrificing correctness for performance is always a losing proposition in the long run. |
|
Well, by using my tool I know I can't use the static config, and the default way of doing things is still working. The alternative, which is definitely supported, is adding For you this might be a hack, but for me it's making the metadata "more correct". IMHO any hardcoded dependency should be mentioned, optional or no. |
|
I agree that using peerDependenciesMeta avoids the downside I mentioned for the first bullet point. However, in no way are any of the dep categories supposed to be a way to "grant access", so it's dangerous to overload them for that. |
|
ES Modules work along the same principle. You can only use import/export statements at package top-levels, for efficiency. All I'm asking is that, for efficiency, the package manifest includes mentions of all dependencies that it will definitely ask for at some point (but might not get). |
|
That's not for efficiency at all, and there was a proposal at one time to allow them nested - and |
|
Oh, I thought it was to allow scanning without executing. So I guess that argument doesn't convince you :-) |
|
I suppose I'm out of arguments. I can only plead for a small mention of a harmless string, which encodes a truth, which you prefer to leave covered. |
|
Yes, but that's not for efficiency, that's for being able to statically know the dep graph before evaluating code :-) (for correctness - it actually is part of the reason why ESM is so much slower than CJS) I do appreciate the persistence with trying multiple arguments. |
|
If you reopen the PR, you'll find an extra commit that makes it super simple for you to push the squash merge button to add |
|
And how are you planning to handle utils/resolve.js, which has its |
|
those are dynamic and therefore will just work for npm-adherents and purists like me will have to provide full paths to resolve plugins. Actually, by editing the package-lock file to add the dep, the build was working for me, so I assume that I already did everything I needed in my case. |
There was a problem hiding this comment.
Given that this package is named "eslint-module-utils", it theoretically won't ever be present in a tree that lacks eslint, and given that marking it as an optional peer dep without marking it also as a peer dep should have no negative consequences for users of proper npm, i'll accept this
Codecov Report
@@ Coverage Diff @@
## main #2523 +/- ##
=======================================
Coverage 95.17% 95.17%
=======================================
Files 66 66
Lines 2798 2798
Branches 940 940
=======================================
Hits 2663 2663
Misses 135 135 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This allows it to find eslint from module-require.js when the package manager is strict.