Skip to content
This repository has been archived by the owner. It is now read-only.

Lock missing dep fixes #17508

Merged
merged 31 commits into from Jun 30, 2017
Merged

Lock missing dep fixes #17508

merged 31 commits into from Jun 30, 2017

Conversation

@iarna
Copy link
Contributor

@iarna iarna commented Jun 29, 2017

This has a handful of fixes:

  1. It introduces a new package-lock.json field, called requires, which tracks which modules a given module requires.
  2. It fixes #16839 which was caused by not having this information available, particularly when git dependencies were involved.
  3. It fixes #16866, allowing the package.json to trump the package-lock.json.
  4. It stops calling fetch-package-metadata.addBundled on fake children (this was harmless, because bundleDependencies would never be set, but also pointless.)
  5. npm ls now loads the shrinkwrap, which opens the door to showing a full tree of dependencies even when nothing is yet installed. (It doesn't do that yet though.)
Copy link
Contributor

@zkat zkat left a comment

Added some comments. Nothing really bad jumps out at me. I'll take a closer look once I've gotten some sleep and/or can actually talk through this and test it out.

Loading

@@ -26,7 +26,8 @@ module.exports = function (tree, swdeps, opts, finishInflating) {
if (!npm.config.get('shrinkwrap') || !npm.config.get('package-lock')) {
return finishInflating()
}
tree.loaded = true
tree.loaded = false
tree.hasRequiresFromLock = Object.keys(swdeps).every((d) => swdeps[d].requires)
Copy link
Contributor

@zkat zkat Jun 29, 2017

Choose a reason for hiding this comment

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

shouldn't this be .some()? You only need to know if there was one, right?

Loading

Copy link
Contributor Author

@iarna iarna Jun 29, 2017

Choose a reason for hiding this comment

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

well... no, if any of them are missing then we can't assume we know anything about the relationships between the modules.

Loading

Copy link
Contributor Author

@iarna iarna Jun 29, 2017

Choose a reason for hiding this comment

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

At one point during the process of writing this it was possible to produce an install that only had partial resolved information. Buuut…

That doesn't seem to be the case any more. So this can become some and we can stop including requires on every node.

Relatedly, I'm finding that npm install xyz or npm rm xyz when you have a legacy package-lock but nothing installed fails to properly upgrade the package-lock, eg:

$ node ~/code/npm rm null

added 190 packages and removed 1 package in 6.671s

Loading

@@ -6,113 +6,163 @@
"abbrev": {
"version": "1.1.0",
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.0.tgz",
"integrity": "sha1-0FVMIlZjbi9W58LlrRg/hZQo2B8="
"integrity": "sha1-0FVMIlZjbi9W58LlrRg/hZQo2B8=",
"requires": {}
Copy link
Contributor

@zkat zkat Jun 29, 2017

Choose a reason for hiding this comment

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

wait, was your solution to unconditionally add a requires:{}? Mehhhh. I don't like this kinda noop crud :\

Loading

Copy link
Contributor Author

@iarna iarna Jun 29, 2017

Choose a reason for hiding this comment

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

Well, I'm open to alternatives.

Loading

@@ -133,13 +127,28 @@ function shrinkwrapDeps (deps, top, tree, seen) {
}
if (childIsOnlyDev) pkginfo.dev = true
if (isOptional(child)) pkginfo.optional = true
pkginfo.requires = {}
Copy link
Contributor

@zkat zkat Jun 29, 2017

Choose a reason for hiding this comment

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

meh @ always having requires

Loading

if (child.fakeChild) {
dep.missing = true
dep.optional = child.package._optional
dep.requiredBy = child.package._spec
Copy link
Contributor

@zkat zkat Jun 29, 2017

Choose a reason for hiding this comment

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

I don't get this line...

Loading

Copy link
Contributor Author

@iarna iarna Jun 29, 2017

Choose a reason for hiding this comment

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

This is where we're producing read-installed's output for ls to consume. In it's format requiredBy is the spec that required the missing dep.

Loading

_resolved: adaptResolved(requested, sw.resolved),
_requested: requested,
_optional: sw.optional,
_integrity: sw.integrity,
_from: from,
_spec: requested.rawSpec,
_where: topPath,
_args: [[requested.toString(), topPath]]
_args: [[requested.toString(), topPath]],
dependencies: sw.requires
Copy link
Contributor

@zkat zkat Jun 29, 2017

Choose a reason for hiding this comment

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

this feels so mischievous 😈

Loading

@iarna iarna force-pushed the lock-missing-dep-fixes branch 2 times, most recently from 508cc07 to a4091fb Jun 29, 2017
@zkat
Copy link
Contributor

@zkat zkat commented Jun 29, 2017

@iarna when I use npm ls, I notice that we get all those nice unmet dependencies, which is awesome. There's two things, tho:

  1. All unmet dependencies are reported as required by the root package, not by their actual logical parent.
  2. I feel like it's redundant for us to spit out individual errors/warnings when we're already printing the tree that shows exactly which ones are missing/invalid/etc. We could probably just collapse that all into a single warning/error that says "some dependencies are missing or invalid" before doing exit 1.

Loading

@zkat zkat force-pushed the lock-missing-dep-fixes branch from f7534bc to e511aa8 Jun 29, 2017
@iarna
Copy link
Contributor Author

@iarna iarna commented Jun 29, 2017

@zkat If it's showing you anything beyond the first level that's a bug. (It doesn't do that for me in my examples…)

We can bikeshed the details of how this looks different showing the full tree later, imo.

Loading

Copy link
Contributor

@legodude17 legodude17 left a comment

📦

Loading

#### dependencies

The dependencies of this dependency, exactly as at the top level.
Exactly like `dependencies` at the top level, this is a list of modules to
isntall in the `node_modules` of this module.
Copy link
Contributor

@legodude17 legodude17 Jun 29, 2017

Choose a reason for hiding this comment

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

Is that supposed to say isntall?

Loading

Copy link
Contributor Author

@iarna iarna Jun 30, 2017

Choose a reason for hiding this comment

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

nope, ty

Loading

@@ -28,15 +28,6 @@ The version of the package this is a package-lock for. This must match what's in
An integer version, starting at `1` with the version number of this document
whose semantics were used when generating this `package-lock.json`.

### packageIntegrity *(new)*
Copy link
Contributor

@legodude17 legodude17 Jun 29, 2017

Choose a reason for hiding this comment

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

Why was this removed? I am confused.

Loading

Copy link
Contributor Author

@iarna iarna Jun 30, 2017

Choose a reason for hiding this comment

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

This was a hash of the package.json which would result in churn and merge conflicts on the package-lock.json. It was never implemented and the consensus of the discussion is that it was a bad idea.

Loading

@@ -85,7 +85,7 @@ test('shrinkwrap uses resolved with file: on local deps', function (t) {
t.comment(stderr.trim())
t.equal(code, 0, 'npm exited normally')
var data = fs.readFileSync(path.join(testdir, 'npm-shrinkwrap.json'), { encoding: 'utf8' })
t.deepEqual(
t.like(
Copy link
Contributor

@legodude17 legodude17 Jun 29, 2017

Choose a reason for hiding this comment

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

Is this an update to tap or a design choice?

Loading

Copy link
Contributor Author

@iarna iarna Jun 30, 2017

Choose a reason for hiding this comment

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

deepEqual requires exact matching of the datastructures. like let's you compare an incomplete datastructure against a complete one. Where they share keys they have to match.

So this change was made to allow changes to the package-lock format that aren't related to this test to happen w/o breaking the test.

Loading

zkat
zkat approved these changes Jun 30, 2017
Copy link
Contributor

@zkat zkat left a comment

LGTM! 💯 🐑 🚀 💥 🔥 😱

Loading

iarna added 18 commits Jun 30, 2017
Also prune before we save, as we skipped the earlier pruning and we don't
want to save a bogus `package-lock`.

Fixes: #16839
This removes a no longer appropriate check of `fromShrinkwrap` and
a duplicated code path when using `_from`.
That is, if you have a package-lock but haven't installed anything yet then
we're going to insist that all of the bits be there. This is necessary in order
to be able to cleanly upgrade old package-lock files. This means if you type
`npm install foo` or `npm rm foo` and don't have a `node_modules` but do have
a `package-lock.json` it's gonna install everything from the
`package-lock.json` in addition to making your change.

The one thing we won't do is update your package-lock from your
package.json.  We only do that when you request a full install.

This does not change the behavior if you don't have a lock file.
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
That is, if you have a package-lock but haven't installed anything yet then
we're going to insist that all of the bits be there. This is necessary in order
to be able to cleanly upgrade old package-lock files. This means if you type
`npm install foo` or `npm rm foo` and don't have a `node_modules` but do have
a `package-lock.json` it's gonna install everything from the
`package-lock.json` in addition to making your change.

The one thing we won't do is update your package-lock from your
package.json.  We only do that when you request a full install.

This does not change the behavior if you don't have a lock file.

PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
We were overriding this, so it wasn't an issue, but it can't be right as
_inBundle is a boolean and fromBundle is a package object.

PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
Previously we checked pkg.package.bundleDependencies which isn't available
to fake children.

PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
We walk all deps now.

PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
That is stop filtering based on `--prod`, `--only=dev`, etc
In future this will happen in `diff-trees`

PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
Previously this happened in various places, `inflate-shrinkwrap` and in
`install` in particular.  As of this writing it's still happening in
`install`—but we'll change that soon.

PR-URL: #17508
Credit: @iarna
Reviewed-By: @zkat
zkat added a commit that referenced this issue Jul 5, 2017
zkat added a commit that referenced this issue Jul 5, 2017
@sonicdoe sonicdoe mentioned this pull request Jul 6, 2017
jameswilson added a commit to BluesparkLabs/gulp-frontend-tasks that referenced this issue Nov 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants