Skip to content
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] hapijs/lab repo fails to install #2005

Closed
kanongil opened this issue Oct 21, 2020 · 4 comments
Closed

[BUG] hapijs/lab repo fails to install #2005

kanongil opened this issue Oct 21, 2020 · 4 comments
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release

Comments

@kanongil
Copy link

Current Behavior:

npm install fails to install all dependencies, and installing specific dependency fails with npm error.

npm ERR! Cannot read property 'version' of undefined

From log:

71 verbose stack TypeError: Cannot read property 'version' of undefined
71 verbose stack     at Arborist.[saveIdealTree] (/home/node-15/.nvm/versions/node/v15.0.0/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/reify.js:800:33)
71 verbose stack     at /home/node-15/.nvm/versions/node/v15.0.0/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/reify.js:124:39
71 verbose stack     at async Arborist.reify (/home/node-15/.nvm/versions/node/v15.0.0/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/reify.js:121:5)
71 verbose stack     at async install (/home/node-15/.nvm/versions/node/v15.0.0/lib/node_modules/npm/lib/install.js:40:5)

Expected Behavior:

Npm installs all dependencies, and npm install typescript works.

Steps To Reproduce:

git clone https://github.com/hapijs/lab.git && cd lab
npm install
npm install typescript

Environment:

  • Node: 15.0.0
  • npm: 7.0.2
@kanongil kanongil 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 Oct 21, 2020
@kanongil
Copy link
Author

kanongil commented Oct 21, 2020

While it this issue could be similar to the one in #1998, it appears to be unique since this fails a direct install. Also, if I remove the peerDependenciesMeta entry from package.json, it will work as expected.

It is interesting that the result depends on a third-party package.json property, which npm does not officially support. At least according to https://github.com/npm/cli/blob/v7.0.3/docs/content/configuring-npm/package-json.md.

@ljharb
Copy link
Contributor

ljharb commented Oct 21, 2020

@kanongil npm definitely officially supports peerDependenciesMeta, it just appears to be missing from the docs.

@isaacs
Copy link
Contributor

isaacs commented Oct 21, 2020

The issue here is that, while we do install peerDependencies by default, we don't install optional peerDependencies, as those are used more for plugins and typically the user does not wish for them to be installed.

We do officially support the peerDependenciesMeta[pkgname].optional boolean, but the docs are lagging behind the code, unfortunately. They'll be updated ahead of moving v7.0.0 to GA status on the latest dist-tag.

What's happening here is that we're loading the peer edge ahead of the dev edge, and then skipping it. And since the peer edge is optional, we don't install it. For the root project at least, dev should take priority, I agree, so this is a bug.

@isaacs
Copy link
Contributor

isaacs commented Oct 21, 2020

Also, even if it's not a dev dependency, if you explicitly tell it to install a package, it should do that, even if it's an optional peer.

So, two bugs :)

isaacs added a commit to npm/arborist that referenced this issue Oct 21, 2020
isaacs added a commit to npm/arborist that referenced this issue Oct 21, 2020
@isaacs isaacs removed the Needs Triage needs review for next steps label Oct 21, 2020
isaacs added a commit that referenced this issue Oct 23, 2020
- Ensure that root is added when root.meta is set
- Include all edges in explain() output when a root edge exists
- Do not conflict on meta-peers that will not be replaced
- Install peerOptionals if explicitly requested, or dev

Fix: #1997
Fix: #2000
Fix: #2005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants