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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Aug 2, 2018 •
edited

@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
christoffer-dropbox
christoffer-dropbox reviewed Aug 10, 2018
View reviewed changes
lib/shrinkwrap.js Outdated
@@ -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
Copy link

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

@ithinkihaveacat ithinkihaveacat mentioned this pull request Sep 21, 2018
Merged
@rarkins rarkins mentioned this pull request Sep 27, 2018
Merged
@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 •
edited

@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 •
edited

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 •
edited

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

@jtwebman jtwebman mentioned this pull request Oct 25, 2018
Merged
1 task
@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?

@rifler
Copy link

rifler commented Mar 16, 2019

hmm, I see code from this pr in latest branch

for example:
https://github.com/npm/cli/blob/latest/lib/shrinkwrap.js#L114

@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 npm@6.9.1-next.0 :(

jfirebaugh added a commit to figma/npmcli that referenced this pull request Jul 19, 2019
@jfirebaugh
Fix npm ci with file: dependencies
c1d5025 
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 jfirebaugh mentioned this pull request Jul 19, 2019
Closed
jfirebaugh added a commit to figma/npmcli that referenced this pull request Jul 19, 2019
@jfirebaugh
Fix npm ci with file: dependencies
262c997 
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
@jfirebaugh @isaacs
Fix npm ci with file: dependencies
2fb0509 
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.
@isaacs isaacs mentioned this pull request Jul 25, 2019
Closed
This was referenced Aug 13, 2019
Open
Merged
This was referenced Aug 30, 2019
Open
Open
Merged
@snyk-bot snyk-bot mentioned this pull request Sep 19, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Oct 1, 2019
Open
This was referenced Oct 26, 2019
Open
Open
This was referenced Nov 5, 2019
Open
Open
@snyk-bot snyk-bot mentioned this pull request Nov 14, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Nov 25, 2019
Open
@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.

@robertsLando robertsLando mentioned this pull request Mar 18, 2022
Closed
2 tasks
@nlf nlf deleted the iarna/enoent-on-link-up branch March 28, 2022 16:56
@varmakarthik12 varmakarthik12 mentioned this pull request Nov 30, 2023
Open
@gutwrinch gutwrinch mentioned this pull request Jan 6, 2024
Open
@Kingcrypto38 Kingcrypto38 mentioned this pull request Jan 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsgw larsgw larsgw left review comments

@christoffer-dropbox christoffer-dropbox christoffer-dropbox left review comments

@suttitum suttitum suttitum 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.

None yet