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.

Sign up for GitHub

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

Jump to bottom

update: re-enable updating local packages #73

Closed
wants to merge 16 commits into from
Closed

update: re-enable updating local packages #73

wants to merge 16 commits into from

Conversation

larsgw
Copy link
Contributor

@larsgw larsgw commented Oct 2, 2018 •
edited
Loading

PR npm/npm#11584 removed the possibility of updating local packages (linked with symlinks) with npm update.
Reason was that this functionality didn't work in v3.6.0. However, the system behind local dependencies
has since changed, and I can't reproduce the original error anymore.

Reverts 53cdb96
See discussion in https://npm.community/t/1725

@larsgw larsgw requested a review from a team as a code owner October 2, 2018 18:28
@zkat zkat changed the base branch from latest to release-next November 13, 2018 15:00
@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

I'd like an 👀 from @iarna on this one.

@jmfNodejsUser
Copy link

Hello,
Is this correction now available in 6.4.1 ?

@iarna
Copy link
Contributor

iarna commented Jan 8, 2019

@jmfNodejsUser It is not, it's not yet been merged.

iarna
iarna suggested changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@iarna iarna left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it took me a bit to wrap my head around what this was doing:

This is fixing a problem where npm update won't auto-heal missing links. (The work around is to run npm i instead, which will.)

I approve of this idea, but there are two things that need to be addressed before it lands:

  1. It needs a test (this would have helped us understand what the patch was hoping to achieve)
  2. With this patch and a file: type dep, npm outdated -l crashes, this'll need to be fixed.

@jmfNodejsUser
Copy link

Thank you for the reply iarna.
How can I help to test this change? OR will somebody from the leader team test the modification?

Your suggestion to use npm i works and installs/links the first/primary dependency (local node module) in the main project. All sub dependencies of the local node modules are not installed/linked. Except when I delete the package-lock.json first and then run "npm i". In this case, it works and installs all primary and sub dependencies which are all local node modules(i.e. not from the npm registry)

@jmfNodejsUser
Copy link

The problem with using "npm i" is the same mentioned in
https://npm.community/t/npm-install-for-package-with-local-dependency-fails/754

@zkat zkat force-pushed the release-next branch 2 times, most recently from 6ca2f43 to 6d0cc95 Compare January 18, 2019 19:17
@larsgw
Copy link
Contributor Author

larsgw commented Jan 18, 2019

Sorry for the confusion. I added tests. As for the npm outdated -l crash, that should be fixed in #124.

@zkat zkat force-pushed the release-next branch 3 times, most recently from db63b89 to b09bc8c Compare January 23, 2019 18:36
zkat and others added 14 commits January 24, 2019 11:24
@zkat
npm-registry-fetch@3.9.0
671cad1 
Make sure publishing with legacy username:password _auth works again
@watilde
doc: use const in lib/fetch-package-metadata.md (npm#120)
89b23a5 
PR-URL: npm#120
Credit: @watilde
Reviewed-By: @aeschright
@watilde @aeschright
misc: replace ronn with marked-man (npm#126)
4970d55 
PR-URL: npm#126
Credit: @watilde
Reviewed-By: @aeschright
@elidoran @aeschright
Reduce work to test if executable ends with a 'g'. (npm#138)
d9b6090 
PR-URL: npm#138
Credit: @elidoran
Reviewed-By: @aeschright
@larsgw @aeschright
test(maketest): Fix common.npm callback arguments (npm#144)
cdb0592 
PR-URL: npm#144
Credit: @larsgw
Reviewed-By: @aeschright
@sokra @aeschright
install/dedupe: fix hoisting of packages with peerDeps (npm#147)
91314e7 
PR-URL: npm#147
Fixes: https://npm.community/t/packages-with-peerdependencies-are-incorrectly-hoisted/4794

Credit: @sokra
Reviewed-By: @aeschright
@larsgw @aeschright
ls: show installed but unmet peer deps (npm#145)
25573e9 
PR-URL: npm#145
Fixes: https://npm.community/t/4770

Credit: @larsgw
Reviewed-By: @aeschright
@greysteil @aeschright
doc: update package.json docs to include repository.directory details (…
3663cde 
…npm#140)

PR-URL: npm#140

Credit: @greysteil
Reviewed-By: @aeschright
@zkat @aeschright
token: use figgy-config to make sure extra opts are there (npm#149)
ce2c4bd 
PR-URL: npm#149

Credit: @zkat
Reviewed-By: @aeschright
pacote@9.4.1
95ca1ae
chore: add changelog for v6.8.0
dacc47d
update AUTHORS
f28a94c
6.8.0-next.0
309260d
@zkat @aeschright
access: ls-collaborators is ok with non-scoped (npm#151)
3c22d1a 
PR-URL: npm#151
Fixes: https://npm.community/t/npm-6-6-0-breaks-access-to-ls-collaborators/5101

Credit: @zkat
Reviewed-By: @aeschright
@jmfNodejsUser
Copy link

Hello @zkat , @iarna ,
Has this correction been merged into version 6.7.0 release?
I tried downloading version 6.7.0 and testing it.
The "npm update" still does not work for local node modules.

@larsgw ,
This problem started after npm version 5.6.0 and exists till now. So I think, that the change in v3.6.0 is probably not the reason for these problems since 5.6.0.
The workaround by using "npm install" still needs a deletion of the package-lock.json.

@larsgw
update: re-enable updating local packages
a853ac7 
PR #11584 removed the possibility of updating local
packages (linked with symlinks) with `npm update`.
Reason was that this functionality didn't work in
v3.6.0. However, the system behind local dependencies
has since changed, and I can't reproduce the original
error anymore.

Reverts 59e5056
@larsgw
test: update & outdated local packages
4d16575
@zkat zkat added semver:patch semver patch level for changes and removed needs-discussion labels Feb 11, 2019
zkat
zkat approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for fixing things up. This looks like it fulfill's @iarna's requirements, so we're good to go. 👍

zkat pushed a commit that referenced this pull request Feb 18, 2019
@larsgw @zkat
update: re-enable updating local packages
e135c2b 
PR #11584 removed the possibility of updating local
packages (linked with symlinks) with `npm update`.
Reason was that this functionality didn't work in
v3.6.0. However, the system behind local dependencies
has since changed, and I can't reproduce the original
error anymore.

Reverts 59e5056

Fixes: https://npm.community/t/1725?u=larsgw
PR-URL: #73
Credit: @larsgw
Reviewed-By: @iarna
Reviewed-By: @zkat
@zkat
Copy link
Contributor

zkat commented Feb 18, 2019

This has been merged by hand. Thanks again, @larsgw!

@zkat zkat closed this Feb 18, 2019
@snyk-bot snyk-bot mentioned this pull request Jun 16, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iarna iarna iarna requested changes

@zkat zkat zkat approved these changes

Assignees
No one assigned
Labels
semver:patch semver patch level for changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@larsgw @zkat @jmfNodejsUser @iarna @watilde @elidoran @sokra @greysteil