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

install: fix install checks for optional/dev dependencies #76

Closed
wants to merge 6 commits into from

Conversation

larsgw
Copy link
Contributor

@larsgw larsgw commented Oct 8, 2018

Problem: when a sub-dependency is required by an optional dependency in two different ways (e.g. once directly and once via another sub-dependency) the sub-dependency checking process ends up at the (optional) parent dependency twice, always failing the check.

Fix: make sure that the tracking of already seen packages has no effects on parallel paths.

See https://npm.community/t/2569

@larsgw larsgw requested a review from a team as a code owner October 8, 2018 14:00
@larsgw larsgw changed the title install: fix install checks for optional dependencies install: fix install checks for optional/dev dependencies Oct 8, 2018
@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

I'd like to get a review from @iarna on this one.

@surmacz
Copy link

surmacz commented Nov 23, 2018

In which version this this fix is going to be released?

@mramato
Copy link

mramato commented Dec 11, 2018

I'm not one for "me-too" posts, but I was extremely disappointed to see this didn't go into npm 6.5.0. It's preventing me and my team from making effective use of package-lock.json or submitting it to our source control.

Is there anything the community can do to help move this patch along? As an OSS maintainer, I completely understand that things can sometimes fall through the cracks when it comes to waiting for someone to review them, but this seems like a fairly minimal, high-impact change to the source.

@iarna
Copy link
Contributor

iarna commented Dec 20, 2018

This was merged as 1342071 and will be in the next release of npm (which will be the first or second week of January, schedules depending).

I rewrote the test to be a bit more in our current style and take advantage of our test tooling. You can see the changes here:

https://gist.github.com/iarna/ca94b7b044cd55c95753964aad27e4d2

@rooby
Copy link

rooby commented Jan 29, 2019

None of the recent release notes seem to have a link to this pull request. Did it make it into 6.6.0 or 6.7.0?

Was it this one in 6.6?

1342071 npm.community#2569 Fix checking for optional dependencies. (@larsgw)

@andersk
Copy link
Contributor

andersk commented Jan 30, 2019

@rooby Yes, see the previous comment.

@khitrenovich
Copy link

Is there any chance that npm v6.6 will be bundled with the next Node v10.x release? Say, v10.16...

@larsgw
Copy link
Contributor Author

larsgw commented Feb 14, 2019

@khitrenovich it's being worked on: nodejs/node#25804

zypA13510 added a commit to zypA13510/ui5-fontawesome that referenced this pull request Jun 7, 2019
This would avoid generating excessive package-lock.json commits (fixed in npm/cli#76).
zypA13510 added a commit to zypA13510/ui5-fontawesome that referenced this pull request Jun 7, 2019
This would avoid generating excessive package-lock.json commits (fixed in npm/cli#76).
lkiesow added a commit to lkiesow/opencast that referenced this pull request Jun 19, 2019
This patch updates node and npm to the latest LTS version which [should
hopefully fix the package-lock.json problem](npm/cli#76 (comment))
although this is hard to verify.

Worst case, we just upgraded npm.
larsgw added a commit to larsgw/cli that referenced this pull request Jul 1, 2019
Instead of creating a new set each time a new node gets visited, so that 
its siblings do not have it in `seen`, just remove the node from the 
original set right after all child nodes are visited.

See npm#76
isaacs pushed a commit that referenced this pull request Jul 2, 2019
Instead of creating a new set each time a new node gets visited, so that
its siblings do not have it in `seen`, just remove the node from the
original set right after all child nodes are visited.

See #76

Credit: @larsgw

PR-URL: #206
Close: #206
Reviewed-by: @isaacs
isaacs pushed a commit that referenced this pull request Jul 3, 2019
Instead of creating a new set each time a new node gets visited, so that
its siblings do not have it in `seen`, just remove the node from the
original set right after all child nodes are visited.

See #76

Credit: @larsgw

PR-URL: #206
Close: #206
Reviewed-by: @isaacs
antongolub pushed a commit to antongolub-forks/npm-cli that referenced this pull request May 18, 2024
* chore: bump @npmcli/template-oss from 4.12.0 to 4.12.1

Bumps [@npmcli/template-oss](https://github.com/npm/template-oss) from 4.12.0 to 4.12.1.
- [Release notes](https://github.com/npm/template-oss/releases)
- [Changelog](https://github.com/npm/template-oss/blob/main/CHANGELOG.md)
- [Commits](npm/template-oss@v4.12.0...v4.12.1)

---
updated-dependencies:
- dependency-name: "@npmcli/template-oss"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore: postinstall for dependabot template-oss PR

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: npm CLI robot <npm-cli+bot@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants