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

Fix isInLink tracking and improve recording of transitive deps file: in locks #40

Closed
wants to merge 4 commits into from

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Aug 2, 2018

@iarna iarna requested a review from a team as a code owner August 2, 2018 07:47
@zkat zkat added semver:patch semver patch level for changes in-progress labels Aug 3, 2018
@@ -110,11 +110,13 @@ function shrinkwrapDeps (deps, top, tree, seen) {
var childIsOnlyDev = isOnlyDev(child)
var pkginfo = deps[moduleName(child)] = {}
var requested = getRequested(child) || child.package._requested || {}
var linked = child.isLink || child.isInLInk

Choose a reason for hiding this comment

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

var linked = child.isLink || child.isInLInk

child.isInLink?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

child.isLink = "This module is a symlink in your node_modules

child.isInLink = "Some parent module of this module is a symlink"

Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a typo with a capital i in Link in isInLink.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was @christoffer-dropbox who caught it, I just wanted to clarify what I thought he meant.

@zkat zkat force-pushed the release-next branch 4 times, most recently from 512c1d5 to f8396dd Compare August 23, 2018 01:24
@ElvishJerricco
Copy link

Is this going to merged? This seems to be required for file: dependencies to have their own package-lock.json files.

@ghost
Copy link

ghost commented Sep 19, 2018

With this fix, it is possible to install local packages with dependencies. However, a package-lock.json is not created and an existing one has no effect for those dependencies:
https://npm.community/t/npm-install-does-not-install-transitive-dependencies-of-local-dependency/2264/2?u=cbuchacher

@johnrjj
Copy link

johnrjj commented Oct 2, 2018

How can I help get this PR merged ?

@ElvishJerricco
Copy link

I would also like to help get this merged. Should I just look at why it's failing CI?

@jtwebman
Copy link
Contributor

jtwebman commented Oct 23, 2018

@zkat What do we need to do to get this PR in the next npm release. We are having to run npm 5.6.0 with node 10 till this is fixed as you can't use file:../any folder in projects.

@jtwebman
Copy link
Contributor

jtwebman commented Oct 25, 2018

FYI, I rebased this branch on latest and ran it locally it does fix the issue and I didn't have any other issues with it. Will run it on my local for awhile to see how well it does on my other projects as well. Not sure if that is what @zkat means by needs testing. I don't have access in Travis CI to re-run but @iarna there are a few issues with the tests. https://travis-ci.com/npm/cli/jobs/145118084 Maybe this breaks something else.

@jtwebman
Copy link
Contributor

I also can run npm test after rebasing on latest and get no failures for this branch.

@jtwebman
Copy link
Contributor

jtwebman commented Oct 25, 2018

Adding some tests now for this issue. Maybe that is what they want. See PR #86

@jtwebman
Copy link
Contributor

@iarna If you want to pull the test from my PR and merge or rebase this on latest I can close my PR. Not trying to take credit just trying to get this merged soon.

@ghost
Copy link

ghost commented Oct 26, 2018

I think this fix is not correct. Please see #40 (comment) above:
https://npm.community/t/npm-install-does-not-install-transitive-dependencies-of-local-dependency/2264

@johnrjj
Copy link

johnrjj commented Oct 30, 2018

@iarna -- Anything we can do to land this PR?

@larsgw
Copy link
Contributor

larsgw commented Mar 16, 2019

Code from this PR was merged as part of another PR, #86.

@iarna
Copy link
Contributor Author

iarna commented Mar 18, 2019

As @larsgw points out, this has actually been merged, so I'm gonna close this now. =D

@iarna iarna closed this Mar 18, 2019
@aaronArinder
Copy link

Thanks for this PR, @iarna: super, super useful. 👍

@lkaratun
Copy link

Still having this issue on [email protected] :(

jfirebaugh added a commit to figma/npmcli that referenced this pull request Jul 19, 2019
Partially reverts npm#40/npm#86, keeping the "Don't record linked deps as bundled" part but reverting the "Don't iterate into linked deps" part. It seems that we need to record dependencies of linked deps in order for `npm ci` to work.

Fixes https://npm.community/t/6-8-0-npm-ci-fails-with-local-dependency/5385
Fixes https://npm.community/t/npm-ci-fail-to-local-packages/6076
jfirebaugh added a commit to figma/npmcli that referenced this pull request Jul 19, 2019
Partially reverts npm#40/npm#86, keeping the "Don't record linked deps as bundled" part but reverting the "Don't iterate into linked deps" part. It seems that we need to record dependencies of linked deps in order for `npm ci` to work.

Fixes https://npm.community/t/6-8-0-npm-ci-fails-with-local-dependency/5385
Fixes https://npm.community/t/npm-ci-fail-to-local-packages/6076
isaacs pushed a commit that referenced this pull request Jul 21, 2019
Partially reverts #40/#86, keeping the "Don't record linked deps as
bundled" part but reverting the "Don't iterate into linked deps" part.
It seems that we need to record dependencies of linked deps in order for
`npm ci` to work.

Fix: https://npm.community/t/6-8-0-npm-ci-fails-with-local-dependency/5385
Fix: https://npm.community/t/npm-ci-fail-to-local-packages/6076
PR-URL: #216
Credit: @jfirebaugh
Close: #216
Reviewed-by: @isaacs

EDIT: Updated test to not rely on network and follow latest and greatest
test patterns.
@spaceneenja
Copy link

It's 2020 and I'm down this rabbit hole on 6.13.4 and I think I'm going to pull the trigger and switch to Yarn.

@nlf nlf deleted the iarna/enoent-on-link-up branch March 28, 2022 16:56
antongolub pushed a commit to antongolub-forks/npm-cli that referenced this pull request May 18, 2024
Bumps [read-cmd-shim](https://github.com/npm/read-cmd-shim) from 2.0.0 to 3.0.0.
- [Release notes](https://github.com/npm/read-cmd-shim/releases)
- [Changelog](https://github.com/npm/read-cmd-shim/blob/main/CHANGELOG.md)
- [Commits](npm/read-cmd-shim@v2.0.0...v3.0.0)

---
updated-dependencies:
- dependency-name: read-cmd-shim
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.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.