Lock missing dep fixes by iarna · Pull Request #17508 · npm/npm

iarna · 2017-06-29T06:33:56Z

This has a handful of fixes:

It introduces a new package-lock.json field, called requires, which tracks which modules a given module requires.
It fixes Missing dependencies after running npm install a second time #16839 which was caused by not having this information available, particularly when git dependencies were involved.
It fixes package-lock.json file not updated after package.json file is changed #16866, allowing the package.json to trump the package-lock.json.
It stops calling fetch-package-metadata.addBundled on fake children (this was harmless, because bundleDependencies would never be set, but also pointless.)
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.)

zkat

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.

zkat · 2017-06-29T06:38:41Z

lib/install/inflate-shrinkwrap.js

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

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

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

zkat · 2017-06-29T06:44:47Z

package-lock.json

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

Well, I'm open to alternatives.

zkat · 2017-06-29T06:45:35Z

lib/shrinkwrap.js

meh @ always having requires

zkat · 2017-06-29T06:47:18Z

lib/install/mutate-into-logical-tree.js

I don't get this line...

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.

zkat · 2017-06-29T06:48:02Z

lib/install/inflate-shrinkwrap.js

this feels so mischievous 😈

zkat · 2017-06-29T16:23:58Z

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

All unmet dependencies are reported as required by the root package, not by their actual logical parent.
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.

iarna · 2017-06-29T19:06:45Z

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

legodude17

📦

legodude17 · 2017-06-29T20:59:24Z

doc/spec/package-lock.md


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


Is that supposed to say isntall?

legodude17 · 2017-06-29T20:59:58Z

doc/spec/package-lock.md

 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)*


Why was this removed? I am confused.

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.

legodude17 · 2017-06-29T21:07:09Z

test/tap/shrinkwrap-local-dependency.js

      t.equal(code, 0, 'npm exited normally')
      var data = fs.readFileSync(path.join(testdir, 'npm-shrinkwrap.json'), { encoding: 'utf8' })
-      t.deepEqual(
+      t.like(


Is this an update to tap or a design choice?

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.

zkat

LGTM! 💯 🐑 🚀 💥 🔥 😱

Fixes: #16839

Also prune before we save, as we skipped the earlier pruning and we don't want to save a bogus `package-lock`. Fixes: #16839

Fixes: #16839

This removes a no longer appropriate check of `fromShrinkwrap` and a duplicated code path when using `_from`.

Fixes: #16839

…quires Fixes: #16839

Fixes: #16839

Fixes: #16866

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.

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

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

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

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

@iarna

Previously we checked pkg.package.bundleDependencies which isn't available to fake children. PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

We walk all deps now. PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

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

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

…ge metadata PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

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

@iarna

…an action for them PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

@iarna

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

See npm/npm#17508 for details

zkat reviewed Jun 29, 2017

View reviewed changes

iarna force-pushed the lock-missing-dep-fixes branch 2 times, most recently from 508cc07 to a4091fb Compare June 29, 2017 09:13

zkat force-pushed the lock-missing-dep-fixes branch from f7534bc to e511aa8 Compare June 29, 2017 19:04

legodude17 reviewed Jun 29, 2017

View reviewed changes

iarna force-pushed the lock-missing-dep-fixes branch 2 times, most recently from 79c4628 to 85b6908 Compare June 30, 2017 01:42

yyjdelete mentioned this pull request Jun 30, 2017

[npm5]npm i with package moved lead to an incorrect package.json in node_modules, which cause Cannot find module #17267

Closed

13 tasks

iarna force-pushed the lock-missing-dep-fixes branch from 85b6908 to fc2ce64 Compare June 30, 2017 08:39

zkat approved these changes Jun 30, 2017

View reviewed changes

iarna added 18 commits June 30, 2017 16:11

inflate-shrinkwrap: Stop calling addBundle on locked deps

9ee7926

refresh-package-json: Children aren't fake after reading real pjson data

5d53397

Fixes: #16839

install: Don't prune if any children are fake

4d824d3

Also prune before we save, as we skipped the earlier pruning and we don't want to save a bogus `package-lock`. Fixes: #16839

shrinkwrap: Save requires information to shrinkwrap/lock

082b0a7

Fixes: #16839

install/deps: Cleanup doesChildVersionMatch implementation

69c07f2

This removes a no longer appropriate check of `fromShrinkwrap` and a duplicated code path when using `_from`.

install/deps: Fakechildren w/o requires always count as semver matches

6ae7a65

Fixes: #16839

install/deps: If we can't find a dependecies match, try the lock's re…

a41fc93

…quires Fixes: #16839

inflate-shrinkwrap: Inflate dependencies from shrinkwrap

55dae35

Fixes: #16839

inflate-shrinkwrap: Let the installer fix invalid lockfiles

57f558d

Fixes: #16866

spec/package-lock: packageIntegrity is no longer a thing

e47dbdc

spec/package-lock: Document requires

adafcc1

ls: Take shrinkwrap into account in ls

7fc8efa

shrinkwrap: Only include requires when something was required

9addc59

install: Guard against missing ideal tree

e14ca8c

install: Include flag at top if the lock contains requires

8d2149f

tests: Make more lenient

715e157

test: get rid of pointless ifError calls in ls-production-and-dev

efd84a2

zkat pushed a commit that referenced this pull request Jul 5, 2017

shrinkwrap: Only include requires when something was required

901bef0

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

install: Guard against missing ideal tree

8dd3337

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

install: Include flag at top if the lock contains requires

ab681e5

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

tests: Make more lenient

db8cfbb

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

test: get rid of pointless ifError calls in ls-production-and-dev

07cefb1

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

test: Update fixture lockfile formats

e8f02f3

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

shrinkwrap: Move is-only-optional and is-only-dev out to their own files

e6d3fd8

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

extract: Read bundles if any children are fromBundle

3e65595

Previously we checked pkg.package.bundleDependencies which isn't available to fake children. PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

deps: Stop avoiding walking shrinkwrapped dev deps

a6865e5

We walk all deps now. PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

inflate-shrinkwrap: Stop filtering deps here

f87ae51

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 pushed a commit that referenced this pull request Jul 5, 2017

inflate-shrinkwrap: Guard against empty/missing dependencies

9bc53f9

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

inflate-shrinkwrap: Make fake children for bundled deps too

f208c0a

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

inflate-shrinkwrap: Standardize how and which _ props we add to packa…

528bedc

…ge metadata PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

install: Only add removed children to summary if there's not already …

8e1bee3

…an action for them PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

zkat pushed a commit that referenced this pull request Jul 5, 2017

package-lock: Update npm's own package lock

e00ee0d

PR-URL: #17508 Credit: @iarna Reviewed-By: @zkat

karanjthakkar mentioned this pull request Jul 6, 2017

ncu doesn't update package-lock.json raineorshine/npm-check-updates#360

Closed

sonicdoe mentioned this pull request Jul 6, 2017

Fix typo #17634

Merged

devinivy mentioned this pull request Jul 15, 2017

Just ignorin the package-lock BigRoomTechnologies/strangeluv#200

Closed

auskast mentioned this pull request Aug 2, 2017

Hoisting seems broken with npm5 lerna/lerna#856

Closed

kreig303 mentioned this pull request Sep 1, 2017

NPM Docs: package-lock.json missing description of "requires". #18356

Open

jameswilson added a commit to BluesparkLabs/gulp-frontend-tasks that referenced this pull request Nov 8, 2017

New dependency tree info from npm 5.1.0

d657f6f

See npm/npm#17508 for details

VickyKoblinski mentioned this pull request Nov 17, 2017

Manually tweaking of package.json should be discouraged survivejs/webpack-book#285

Closed

fkleuver mentioned this pull request Jan 17, 2018

Second and following npm i fails when npm 3.10.10 present in the dependency tree #19304

Open

2 tasks

pmdartus mentioned this pull request Jan 24, 2018

Update testing dependencies postcss/postcss-selector-parser#128

Merged

chenxsan mentioned this pull request Mar 9, 2018

Upgrade expose-loader to 0.7.5 phoenixframework/phoenix#2803

Merged

AlanGabbianelli mentioned this pull request Sep 27, 2018

Remove policies related code alphagov/govuk_publishing_components#540

Merged

samelhusseini mentioned this pull request Mar 29, 2021

package-lock.json mismatch RaspberryPiFoundation/blockly-samples#632

Open

Conversation

iarna commented Jun 29, 2017

Uh oh!

zkat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkat commented Jun 29, 2017

Uh oh!

iarna commented Jun 29, 2017

Uh oh!

legodude17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants