-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
[BUG] Locally-installed package should satisfy peer dependency #2199
Labels
Bug
thing that needs fixing
Priority 0
will get attention right away
Release 7.x
work is associated with a specific npm 7 release
Milestone
Comments
btmills
added
Bug
thing that needs fixing
Needs Triage
needs review for next steps
Release 7.x
work is associated with a specific npm 7 release
labels
Nov 19, 2020
1 task
I get a similar warning on the latest npm version:
|
@RomanHotsiy try v7.4.0? |
@ljharb same. |
darcyclarke
added
Priority 1
high priority issue
Enhancement
new feature or improvement
and removed
Needs Triage
needs review for next steps
labels
Feb 2, 2021
Shell script to reproduce this problem with the latest npm v7.x: tmp=$(mktemp -d)
echo "tmp dir: $tmp"
mkdir "$tmp/npm"
cd "$tmp/npm"
npm i npm@^7.0.0
npm() { "$tmp"/npm/node_modules/.bin/npm "$@"; }
# Create a dummy ep_etherpad-lite package. Later, a real package
# will be installed that has a peer dependency on this package.
mkdir "$tmp/ep_etherpad-lite"
cd "$tmp/ep_etherpad-lite"
npm init -y
npm version major # Bump version to 2.0.0 (needed below).
# Create dummy main package that depends on the above
# ep_etherpad-lite package via 'file:'.
mkdir "$tmp/main"
cd "$tmp/main"
npm init -y
npm i ep_etherpad-lite@file:../ep_etherpad-lite
# Install ep_readonly_guest in main. ep_readonly_guest has a peer dependency on
# ep_etherpad-lite@>=1.8.7, which is why the version was bumped to 2.0.0 above.
# The installation of ep_readonly_guest fails because npm can't determine if the
# installed ep_etherpad-lite package satisfies ep_readonly_guests's peer
# dependency on ep_etherpad-lite.
cd "$tmp/main"
npm i ep_readonly_guest@1.0.28
rm -rf "$tmp" |
rhansen
added a commit
to ether/etherpad-lite
that referenced
this issue
Feb 16, 2021
This works around npm/cli#2199.
rhansen
added a commit
to ether/etherpad-lite
that referenced
this issue
Feb 16, 2021
The `--no-save` is so that npm v6 does not get confused when it is run during Etherpad startup to discover plugins. The `--legacy-peer-deps` flag works around npm/cli#2199.
rhansen
added a commit
to ether/etherpad-lite
that referenced
this issue
Feb 16, 2021
The `--no-save` is so that npm v6 does not get confused when it is run during Etherpad startup to discover plugins. The `--legacy-peer-deps` flag works around npm/cli#2199.
rhansen
added a commit
to ether/etherpad-lite
that referenced
this issue
Feb 17, 2021
The `--no-save` is so that npm v6 does not get confused when it is run during Etherpad startup to discover plugins. The `--legacy-peer-deps` flag works around npm/cli#2199.
rhansen
added a commit
to ether/etherpad-lite
that referenced
this issue
Feb 17, 2021
The `--no-save` is so that npm v6 does not get confused when it is run during Etherpad startup to discover plugins. The `--legacy-peer-deps` flag works around npm/cli#2199.
rhansen
added a commit
to ether/etherpad-lite
that referenced
this issue
Feb 18, 2021
See: npm/cli#2199 This flag is unknown to npm v6, but npm v6 silently ignores unknown flags.
darcyclarke
added
Bug
thing that needs fixing
Priority 0
will get attention right away
and removed
Priority 1
high priority issue
Bug
thing that needs fixing
Enhancement
new feature or improvement
labels
Mar 1, 2021
isaacs
added a commit
to npm/arborist
that referenced
this issue
Mar 9, 2021
Previously, we were not including link targets in the virtual trees where peer dependency sets are calculated. Additionally, we were still using the path `/virtual-root` for the virtual node, even though this is no longer load-bearing. (And, as of the recent change to the Node printable output, no longer necessary or particularly helpful for debugging.) As a result, a link dependency from the root node like `file:../../foo` would get resolved against `/virtual-root`, resulting in `/foo`, which of course does not match any Node in the virtual tree. The outcome was an ERESOVLVE error where the `current` Node is shown as having no name or version (because it is an unsatisfied Link). The solution is two-part. First, the path of the virtual tree root now matches the path of the Node that it is sourced from. Second, any Link children of the source node have their targets mirrored in the virtual tree, resulting in them being matched appropriately. The result is that a Link dependency can now properly satisfy a peerDependency. Test shows an example of using a Link to a local Node as a workaround for a peerSet that otherwise would not be resolveable. This can of course be abused to get around valid peerDep contracts, but if they user takes it on themselves to use a local fork of a dependency, we should respect that in buildIdealTree as we do elsewhere. Fix: npm/cli#2199
isaacs
added a commit
to npm/arborist
that referenced
this issue
Mar 9, 2021
Previously, we were not including link targets in the virtual trees where peer dependency sets are calculated. Additionally, we were still using the path `/virtual-root` for the virtual node, even though this is no longer load-bearing. (And, as of the recent change to the Node printable output, no longer necessary or particularly helpful for debugging.) As a result, a link dependency from the root node like `file:../../foo` would get resolved against `/virtual-root`, resulting in `/foo`, which of course does not match any Node in the virtual tree. The outcome was an ERESOLVE error where the `current` Node is shown as having no name or version (because it is an unsatisfied Link). The solution is two-part. First, the path of the virtual tree root now matches the path of the Node that it is sourced from. Second, any Link children of the source node have their targets mirrored in the virtual tree, resulting in them being matched appropriately. The result is that a Link dependency can now properly satisfy a peerDependency. Test shows an example of using a Link to a local Node as a workaround for a peerSet that otherwise would not be resolveable. This can of course be abused to get around valid peerDep contracts, but if they user takes it on themselves to use a local fork of a dependency, we should respect that in buildIdealTree as we do elsewhere. Fix: npm/cli#2199
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Bug
thing that needs fixing
Priority 0
will get attention right away
Release 7.x
work is associated with a specific npm 7 release
Current Behavior:
npm install
in the eslint/eslint repository fails when it tries to resolve a transitive peer dependency oneslint@>=5.0.0
and findseslint@undefined
.We want to dogfood
eslint
in development, so we specify it in ourdevDependencies
as"eslint": "file:."
. We also depend on a third-party plugin,eslint-plugin-eslint-plugin
, which listseslint@>=5.0.0
in itspeerDependencies
.Running
npm install
reportsERESOLVE unable to resolve dependency tree
having foundeslint@undefined
, which fails theeslint@>=5.0.0
requirement. (Full output anderesolve-report.txt
included in the repro steps.)eslint@7.13.0
`spackage.json
snippeteslint-plugin-eslint-plugin@2.3.0
`spackage.json
snippetExpected Behavior:
Ideally, instead of finding
eslint@undefined
,eslint-plugin-eslint-plugin
'seslint@>=5.0.0
peer dependency is satisifed byeslint@7.13.0
, which is already "installed" fromfile:.
. Install continues on as normal. (It won't succeed yet because I'm still working on upgrading another dependency to a newer version that listseslint@^7.0.0
as a compatible peer dependency, buteslint-plugin-eslint-plugin
at least should be happy.)If that's not an option, our current solution so that we can test Node 15 in CI is to install with the
--legacy-peer-deps
flag. I also found that replacingfile:
dependencies withworkspaces
installs successfuly. At some point in the future when we can require at least npm v7, we could switch from--legacy-peer-deps
toworkspaces
.Steps To Reproduce:
~/.npm/eresolve-report.txt
Environment:
The text was updated successfully, but these errors were encountered: