Skip to content

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

Closed
iarna wants to merge 4 commits into
release-nextfrom
iarna/enoent-on-link-up
Closed

Fix isInLink tracking and improve recording of transitive deps file: in locks#40
iarna wants to merge 4 commits into
release-nextfrom
iarna/enoent-on-link-up

Conversation

@iarna

@iarna iarna commented Aug 2, 2018

Copy link
Copy Markdown
Contributor

@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
Comment thread lib/shrinkwrap.js Outdated
var childIsOnlyDev = isOnlyDev(child)
var pkginfo = deps[moduleName(child)] = {}
var requested = getRequested(child) || child.package._requested || {}
var linked = child.isLink || child.isInLInk

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@ghost

ghost commented Sep 19, 2018

Copy link
Copy Markdown

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

johnrjj commented Oct 2, 2018

Copy link
Copy Markdown

How can I help get this PR merged ?

@ElvishJerricco

Copy link
Copy Markdown

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

@jtwebman

jtwebman commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

@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

jtwebman commented Oct 25, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@jtwebman

jtwebman commented Oct 25, 2018

Copy link
Copy Markdown
Contributor

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

@jtwebman

Copy link
Copy Markdown
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

ghost commented Oct 26, 2018

Copy link
Copy Markdown

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

johnrjj commented Oct 30, 2018

Copy link
Copy Markdown

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

@lkaratun

Copy link
Copy Markdown

Still having this issue on npm@6.9.1-next.0 :(

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
Copy Markdown

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] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
koralle pushed a commit to koralle/npm-cli that referenced this pull request Feb 11, 2026
Jah-yee pushed a commit to Jah-yee/cli that referenced this pull request Apr 16, 2026
Jah-yee pushed a commit to Jah-yee/cli that referenced this pull request Apr 16, 2026
…unicode

fix(table): flatten nested objects to dot-notation, safe multi-byte truncation (fixes npm#40 npm#43)
github-actions Bot added a commit to Kevinlee7250/cli that referenced this pull request Jun 8, 2026
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.