npm-shrinkwrap.json handles metadata inconsistently #3581

Closed
shaneholder opened this Issue Jun 18, 2013 · 66 comments

Projects

None yet
@shaneholder

If npm resolves a file from a registry when shrinkwrap is run a "resolved" property is added to the npm-shrinkwrap.json file. If npm finds that the file is in the cache then this property is not written.

npm cache clear
npm init
npm install winston
npm shrinkwrap
check shrinkwrap file and see "resolved" properties
rm -rf node_modules npm-shrinkwrap.json
npm install winston
npm shrinkwrap
check shrinkwrap file and see "resolved" properties do not exist

It seems that npm shrinkwrap should generate the same output regardless of if the file came from the cache or if the file came from the registry. This becomes more important if one is mixing private and public registries as it seems that the resolved property is used to locate a dependency.

@gabrielf
Contributor
gabrielf commented Jul 4, 2013

How does this problem affect you? For us it leads to lots of dependencies being downloaded over and over again.

Example:
A shrinkwrap file for an application that only depends on async can look in the following two ways depending on (as @shaneholder pointed out) whether async was installed form the cache or not.

If async was installed with an empty cache:

{
  "name": "myapp",
  "version": "0.0.0",
  "dependencies": {
    "async": {
      "version": "0.2.9",
      "from": "async@",
      "resolved": "https://registry.npmjs.org/async/-/async-0.2.9.tgz"
    }
  }
}

Or if async was installed from the cache:

{
  "name": "myapp",
  "version": "0.0.0",
  "dependencies": {
    "async": {
      "version": "0.2.9",
      "from": "async@",
    }
  }
}

If the resolved is set then that means the tgz will be downloaded every install regardless whether it's in the cache or not.

@gabrielf
Contributor
gabrielf commented Jul 4, 2013

I suspect the reason is that the package.json for a downloaded dependency looks different when installed from the cache and when it's installed fresh from the central repo. I made two folders and installed async in both, first with an empty cache and then from the cache. A diff between the two folders yield:

diff -r application-no-cache/node_modules/async/package.json application-from-cache/node_modules/async/package.json
41,45c41
<   "dist": {
<     "shasum": "1d805cdf65c236e4b351b9265610066e4ae4185c"
<   },
<   "_from": "async@",
<   "_resolved": "https://registry.npmjs.org/async/-/async-0.2.9.tgz"
---
>   "_from": "async@"
@gabrielf
Contributor
gabrielf commented Jul 4, 2013

I wrote a small, but ugly, shell script that creates a shrinkwrap file that does not download unnecessary modules again on subsequent npm install.

It basically removed all modules from node_modules that experience the problem, runs npm install to make sure they get put in the npm cache. It then deletes problematic modules again and runs node_modules to make sure all modules are installed from the cache. At the end a shrinkwrap file is created where all dependencies have been installed from the cache which mean they will not have to be re-downloaded.

function deleteModulesThatWasInstalledStraightFromCentralRepo() {
  for module in  node_modules/*
  do
    MODULE_PATH="$module"
    if grep -q '"_resolved": "https://registry.npmjs.org' "$MODULE_PATH/package.json" ; then
      echo "Deleting $MODULE_PATH"
      rm -rf $MODULE_PATH
    fi
  done
}

deleteModulesThatWasInstalledStraightFromCentralRepo
rm npm-shrinkwrap.json
npm install
deleteModulesThatWasInstalledStraightFromCentralRepo
npm install
npm shrinkwrap --dev
@aseemk
aseemk commented Oct 24, 2013

Wow, after a lot of head-scratching and debugging, I finally arrived at this same thing. Two symptoms for me:

  1. Subsequent npm installs re-install dependencies. This is particularly bad for large apps as they grow. I discovered this because Heroku is starting to cache node_modules, but even so this kicks in:

    heroku/heroku-buildpack-nodejs@83104b4#commitcomment-4413053

  2. Re-installing (cleanly, i.e. deleting node_modules first) from shrinkwrap then re-shrinkwrapping causes random, undeterministic changes in the shrinkwrap, due to these resolved lines appearing and disappearing.

    You'd expect shrinkwrap to be idempotent!

@mgol
mgol commented Nov 9, 2013

@aseemk Completely agree, I've just been hit by those 2 issues you pointed out.

@vvo
vvo commented Nov 9, 2013

due to these resolved lines appearing and disappearing.

This is so annoying that coworkers suggested to modify shrinkwrap.json manually or removing shrinkwrap.

@aseemk
aseemk commented Nov 12, 2013

I discovered a very simple workaround for now β€” just run this after every shrinkwrap:

var fs = require('fs');
var shrinkwrap = require('./npm-shrinkwrap.json');

function replacer(key, val) {
    if (key === 'resolved' && this.from && this.version) {
        return undefined;
    } else {
        return val;
    }
}

fs.writeFileSync('./npm-shrinkwrap.json', JSON.stringify(shrinkwrap, replacer, 2));

It takes advantage of JSON.stringify()'s replacer argument to recurse into every dependency and remove the resolved keys.

You can run that manually after every shrinkwrap, or wrap it in bash to shrinkwrap too:

npm shrinkwrap
echo '''
    // above JS goes here
''' | node

Hope this helps!

@mgol
mgol commented Nov 12, 2013

@aseemk Thx for the script!

@vvo
vvo commented Nov 18, 2013

I know we should not use npm while deploying BUT the current shrinkwrap problem might worsen the current npm instabilities issues.

If all projects using shrinkwrap are re-downloading packages I guess npm is under big load due to this bug (partly).

@vvo
vvo commented Nov 19, 2013

Funny enough the number of expressjs downloads doubled 5 monthes ago, when this issue was created

selection_066

selection_067

http://npm-stat.vorba.ch/charts.html?package=express

This does not seems to be a summer effect since last year did not see the same raise.

@isaacs
Member
isaacs commented Nov 19, 2013

It's safe to say that this bug is not significantly affecting downloads. We're simply still in the exponential portion of npm's growth curve. Shrinkwrap usage is a rounding error.

That being said, this is indeed a bug, and should be fixed. Those "resolved" fields should be added in the cache data, not just in the installed package.json.

@vvo
vvo commented Dec 4, 2013

I hope too it's not affecting downloads or not too much.

But given every company using npm install and shrinkwrap in deployments and given the hype for "continuous deployment" I thought that could be one of the tiny reason for the raise in npm downloads.

@moll
moll commented Dec 4, 2013

This bug, as might've said above, also affects unpublished modules that were installed manually with npm install *.tgz. Those are still looked up in NPM and therefore cause npm install to fail.

@moll
moll commented Dec 17, 2013

Seems that removing those resolved properties from the shrinkwrap isn't much of a workaround as it'll then not install modules with Git repository sources from the repositories, but still from the NPM repo.

@mgol
mgol commented Dec 17, 2013

@moll Ah, right, that's why my package was misbehaving...

I guess I'll just be making sure that one resolved property remains...

@Raynos
Contributor
Raynos commented Jan 15, 2014

πŸ‘ any suggestions on how to fix the cache to write the resolved fields ?

@hurrymaplelad

Quick regex fix for me. Leaves resolved for git repos.

s/,\n^\s*"from": ".*registry.npmjs.org[^,\n]*(,?$)/$1/
s/,\n^\s*"resolved": ".*registry.npmjs.org[^,\n]*(,?$)/$1/

Were these from and resolved fields pointing at npm adding some value that I'm missing, or should shrinkwrap not generate them in the first place?

@Raynos
Contributor
Raynos commented Feb 5, 2014

Run npm cache clear before rm -rf ./node_modules && npm i && npm shrinkwrap

It will generate from and resolved fields properly.

shinkwrap just puts node_modules into a file. it's npm i that writes inconsistent package.json files into node_modules tree based on install from cache or install from full registry

@hurrymaplelad

Heads up, wrote a quick tool, clingwrap to simplify updating shrinkwrap. Interested in a PR to get this behavior in core npm?

@tinyfrog

if you attempt to remove 'from' and 'resolved' fields from shrinkwrap, be aware that ynpm install would fail if the version of dependent package is a git url instead of a regular semver, e.g.
https://github.com/jshint/jshint/blob/d5f6699a0269e2892629d22cf8e2941bd3374f74/package.json#L25

@hurrymaplelad

@TinyFrog, exactly. We only want to prune from and resolved if they point to registry.npmjs.org.

@mgol mgol referenced this issue in angular/angular.js Mar 12, 2014
@IgorMinar IgorMinar chore(npm): add shrinkwrap to lock down dependencies
We need to be able to build angular at older shas, without the lock file / shrinkwrap file
the dependencies will resolve differently on different machines and at different times.

This will help us avoid broken builds and hard to track down issues.

I had to manually edit this file after it was generated because `npm shrinkwrap` will install
optional dependencies as if they were hard dependencies.

See: npm/npm#2679 (comment)

My manual edit:

```
diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json
index 756df44..dc157eb 100644
--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -3110,19 +3110,7 @@
         "chokidar": {
           "version": "0.8.1",
           "from": "https://registry.npmjs.org/chokidar/-/chokidar-0.8.1.tgz",
-          "resolved": "https://registry.npmjs.org/chokidar/-/chokidar-0.8.1.tgz",
-          "dependencies": {
-            "fsevents": {
-              "version": "0.1.6",
-              "from": "fsevents@0.1.6",
-              "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-0.1.6.tgz"
-            },
-            "recursive-readdir": {
-              "version": "0.0.2",
-              "from": "recursive-readdir@0.0.2",
-              "resolved": "https://registry.npmjs.org/recursive-readdir/-/recursive-readdir-0.0.2.tgz"
-            }
-          }
+          "resolved": "https://registry.npmjs.org/chokidar/-/chokidar-0.8.1.tgz"
         },
         "glob": {
           "version": "3.2.9",
```

Additionally chokidar doesn't list the dependencies above as optional, but that will hopefully
be soon fixed: paulmillr/chokidar#106

In the meantime the patch from the PR above needs to be applied to
node_modules/karma/node_modules/chokidar/package.json before running `npm shrinkwrap`

----

After this change is applied, angular core developers don't need to do anything differently,
except when updating dependencies we need to call `npm update && npm shrinkwrap --dev`
followed by reappling my patch above until npm's bug.

Closes #6653
f684cb0
@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 14, 2014
@IgorMinar IgorMinar chore(npm): clean up shrinkwrap file, remove unused properties
from our experiements it appears that the presense or absense of the from and resolved properties
makes no difference on the behavior of  but  updates these properties
with different values depending on different state of the cache and node_modules.

So in order to get clean diffs during updates, we are just going to drop these properties and have
a script to do this automatically.

Long term this should be fixed in npm: npm/npm#3581
fb8ac28
@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 14, 2014
@petebacondarwin @IgorMinar petebacondarwin + IgorMinar chore(clean-shrinkwrap): add a utility to clean up the shrinkwrap file
This is to deal with npm/npm#3581

See the previous commit for more info.

Closes #6672
b66cdc7
@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 15, 2014
@petebacondarwin @IgorMinar petebacondarwin + IgorMinar chore(clean-shrinkwrap): add a utility to clean up the shrinkwrap file
This is to deal with npm/npm#3581

See the previous commit for more info.

Closes #6672
75e5091
@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 15, 2014
@IgorMinar IgorMinar chore(npm): clean up shrinkwrap file, remove unused properties
from our experiements it appears that the presense or absense of the from and resolved properties
makes no difference on the behavior of  but  updates these properties
with different values depending on different state of the cache and node_modules.

So in order to get clean diffs during updates, we are just going to drop these properties and have
a script to do this automatically.

Long term this should be fixed in npm: npm/npm#3581
e5dd832
@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Mar 15, 2014
@petebacondarwin @IgorMinar petebacondarwin + IgorMinar chore(clean-shrinkwrap): add a utility to clean up the shrinkwrap file
This is to deal with npm/npm#3581

See the previous commit for more info.

Closes #6672
dd3587a
@IgorMinar IgorMinar added a commit to angular/angular.js that referenced this issue Mar 16, 2014
@IgorMinar IgorMinar chore(npm): clean up shrinkwrap file, remove unused properties
from our experiements it appears that the presense or absense of the from and resolved properties
makes no difference on the behavior of  but  updates these properties
with different values depending on different state of the cache and node_modules.

So in order to get clean diffs during updates, we are just going to drop these properties and have
a script to do this automatically.

Long term this should be fixed in npm: npm/npm#3581
c794b96
@petebacondarwin petebacondarwin added a commit to angular/angular.js that referenced this issue Mar 16, 2014
@petebacondarwin @IgorMinar petebacondarwin + IgorMinar chore(clean-shrinkwrap): add a utility to clean up the shrinkwrap file
This is to deal with npm/npm#3581

See the previous commit for more info.

Closes #6672
771bccc
@revolunet revolunet added a commit to revolunet/angular.js that referenced this issue Jun 16, 2014
@IgorMinar @revolunet IgorMinar + revolunet chore(npm): clean up shrinkwrap file, remove unused properties
from our experiements it appears that the presense or absense of the from and resolved properties
makes no difference on the behavior of  but  updates these properties
with different values depending on different state of the cache and node_modules.

So in order to get clean diffs during updates, we are just going to drop these properties and have
a script to do this automatically.

Long term this should be fixed in npm: npm/npm#3581
c06bcb1
@revolunet revolunet added a commit to revolunet/angular.js that referenced this issue Jun 16, 2014
@petebacondarwin @revolunet petebacondarwin + revolunet chore(clean-shrinkwrap): add a utility to clean up the shrinkwrap file
This is to deal with npm/npm#3581

See the previous commit for more info.

Closes #6672
8b330ca
@mcculloughsean

bump Is there going to be an immediate fix for this? Will it be resolved with the improved install process @othiym23 proposed?

@Raynos
Contributor
Raynos commented Aug 1, 2014

You can use a tool like npm-shrinkwrap that is authored & maintained in user land to work around this problem.

@gabegorelick gabegorelick added a commit to gabegorelick/fracas that referenced this issue Sep 2, 2014
@gabegorelick gabegorelick Delete npm-shrinkwrap
It's not ready for primetime. See
npm/npm#3581 and similar issues.
08f7fdb
@cstamas
cstamas commented Oct 9, 2014

What will resolved field contain if I'm behind a private registry? What if the owner of private registry decides to change it's name (and all the URLs along that line)? What if a company, the owner of that private registry is acquired by company B? What if...

Storing URL in metadata (meant to last) is very very bad idea...

As there are already packages out there with URLs, I believe it's the nom that should be made smarter, and make it somehow ignore those (by configuration for example).

@peterlynch

Hardcoding full URLs that could go away at any time inside a dependency metadata is a design flaw with NPM.

If the intent of the resolved field was to force dependencies to be retrieved from a private npm repo, then the cart was put before the horse. The npm client should instead fully control where dependencies are asked from. A package should only define what to ask for (package + version/version range).

Other dependency tools have learned this the hard way. Maven, Gradle, Ivy, etc all give the client full control over where to get artifacts.

Private npm repositories behind firewalls are becoming more common. By hardcoding complete URLs to registry.npmjs.org inside shrinkwrap json, you are forcing npm clients to ignore your shrinkwrap file. Is that a good thing?

A client can ignore the shrinkwrap json file ( and therefore the resolved field ) by using npm c set shrinkwrap false. However, does this not also ignore perhaps useful version specification inside the shrinkwrap file?

Please:

  • remove the ability the set resolved field in shrinkwrap json
  • provide npm client a forced way to always GET packages from a specific registry, no matter what the package metadata suggests
@othiym23
Contributor

If the intent of the resolved field was to force dependencies to be retrieved from a private npm repo, then the cart was put before the horse.

This isn't why resolved is saved within npm-shrinkwrap.json, but as a project we've done a poor job of explaining what it actually is for. The purpose of shrinkwrapping in general is to produce consistent, repeatable builds. Ideally, when you install a shrinkwrapped project, you will deploy the exact same bits that were on disk when you created the shrinkwrap file. The intent is that using shrinkwrap will give you the benefits of checking node_modules into git without the costs.

Other dependency tools have learned this the hard way. Maven, Gradle, Ivy, etc all give the client full control over where to get artifacts.

In general, this is a property that npm shares, in that typical Node applications and libraries use semver ranges to specify dependencies, and can pull those dependencies from whatever sources you as a developer deem appropriate (the npm, Inc. registry, Nodejitsu's registry, a private registry, GitHub, raw tarball URLs). npm 2 even adds finer-grained control over where you get packages from by adding support for scopes. npm shrinkwrap, however, is a special case. When you're shrinkwrapping your app, you're explicitly saying that you want exactly these dependencies to be put on disk. In this case, provenance and consistency are important.

I think of creating a shrinkwrap file for an application as a finishing step; it's one of the last things you do before handing the package off to CI / the staging environment / your deployment framework.

Please:

  • remove the ability the set resolved field in shrinkwrap json
  • provide npm client a forced way to always GET packages from a specific registry, no matter what the package metadata suggests

For reasons that I hope are now clear, we're not going to do this. The use of shrinkwrap files is not required to use npm, and in fact I don't even recommend you use them for libraries. As some of the above discussion attests, I want working with shrinkwrap files to be easier and more central to how npm works, but they will most likely always be a useful deployment tool rather than the dominant way people do their package installs.

@arikon
Contributor
arikon commented Oct 10, 2014

@othiym23 The only reason to use npm shrinkwrap is to stick to specific versions regardles of ranges specified in package.json. But storing full urls to the specific registries ruins that case, because of these registries could be inaccessible or very slow in certain conditions.

The root cause of that problem is people are publishing this npm-shrinkwrap.json with hardcoded urls to registries.

@othiym23
Contributor

But storing full urls to the specific registries ruins that case, because of these registries could be inaccessible or very slow in certain conditions.

I understand, and I apologize if I did a poor job of explaining the implications of storing full URLs: the tarball for a package published to one registry (say, your private registry) may have very different contents from a package published to another (say, the default registry). Especially with the advent of private packages / scoped packages that may vary from one registry to another, we need some way of saying exactly which set of bits to use.

We could store both the exact version of the package and the registry from which we pulled it, but at least with the public registry and npmE, this is substantively the same as just storing the tarball URL.

The root cause of that problem is people are publishing this npm-shrinkwrap.json with hardcoded urls to registries.

That's arguably a poor practice for packaging libraries (as opposed to deployable applications), but I've seen good arguments made for this from maintainers of large, complex packages. Either way, you can get around this by building up your application's dependencies, deleting node_modules, and running npm install --no-shrinkwrap. It's more awkward than it should be, but that's why this issue is open and why the shrinkwrap label exists on this tracker. ;)

@Raynos
Contributor
Raynos commented Oct 10, 2014

The root cause of that problem is people are publishing this npm-shrinkwrap.json with hardcoded urls to registries.

This can cause serious issues.

When running npm install --registry=BLA nothing in node_modules should come from any other registry. Any packages that have an npm-shrinkwrap.json will break that invariant.

npm-shrinkwrap.json is a tool for application developers, not module authors. Opening an issue about npm install ignoring nested npm-shrinkwrap.json files is a good idea.

@peterlynch

This isn't why resolved is saved within npm-shrinkwrap.json, but as a project we've done a poor job of explaining what it actually is for. The purpose of shrinkwrapping in general is to produce consistent, repeatable builds.

Repeatable builds should have nothing to do with where to download something from. If you want to always be sure you have the same dependency, then the hash of the dependency should ultimately be used ( hence why tools like Maven can fail builds based on checksum mismatch ) . Barring that in 99% of cases a id + semver should be good enough from a human point of view.

Simply using a URL does not guarantee anything. Once a request arrives at the URL, the server can decide to serve anything it likes. At the very least the end user should be able to specify a URL they would like to ask for stuff.

Could we agree the official NPM registry should not allow packages uploaded with npm-shrinkwrap.json files containing full URLs?

That's arguably a poor practice for packaging libraries (as opposed to deployable applications), but I've seen good arguments made for this from maintainers of large, complex packages.

Please enumerate the good arguments in a public document.

@domenic
Member
domenic commented Oct 20, 2014

I agree with @peterlynch's points about the integrity of URLs versus checksums. I think they should be seriously considered.

@cstamas
cstamas commented Oct 20, 2014

+1 for @peterlynch points. URLs per-se does not guarantee anything. Next to Peter's point with registries, consider also use of HTTP proxy to connect to registry: you are left on proxy's "good will" to serve you what it wants.

@Raynos
Contributor
Raynos commented Oct 20, 2014

Could we agree the official NPM registry should not allow packages uploaded with npm-shrinkwrap.json files containing full URLs?

πŸ‘

@Raynos
Contributor
Raynos commented Oct 20, 2014

It should also be noted that everybody agrees checksums are better. It's simply a matter of not implemented yet and the URL will stay in the short/mid term.

@othiym23 othiym23 changed the title from npm shrinkwrap does not write resolved property when file comes from cache to npm-shrinkwrap.json handles metadata inconsistently Oct 21, 2014
@othiym23 othiym23 added this to the cache rewrite milestone Oct 21, 2014
@othiym23
Contributor

(I retitled this issue to better capture the nature of the discussion in the comments.)

From a comment I made on #6444:

npm-shrinkwrap.json is a tool for build and release engineers. It can be used by developers, and it may be useful in that capacity, but that's not its primary purpose. It is intended to ensure that an application can be deployed repeatably and consistently, and as such its primary purpose is to make sure that the same bits get put on disk for every user running npm install for a given shrinkwrapped application.

At present, shrinkwrap does a poor job of this. It's a production prototype of a useful feature, and it's on the roadmap as a feature we are committed to improving. That said, the above is my understanding of what problem npm shrinkwrap is trying to solve. If people have a different idea of what its job is, now would be a good time to let me know.

There are many ways to decide whether we have "the same" bits. In npm 2, the ground truth is the where – the resolved URL of the tarball. This is stronger than the semver range, because it accounts for packages with the same version but different contents deployed to different registries.

Stronger still would be to use the what – the hash of the package's contents. This handles the case in which people have the correct version of a package in their caches (which, if used, would save users of npm shrinkwrap a huge amount of time and bandwidth, as it wouldn't need to talk to the registry or download anything once the cache was primed). Unfortunately, it doesn't deal with private registry and enterprise use cases where the ops team republishes dependencies as a matter of course.

So, shasums are a stronger guarantor of identity than URLs, and URLs are stronger than semver. At the same time, some of the other invariants that hold for the public registry don't hold for private or enterprise registries. These are assumptions that npm should honor when installing a shrinkwrapped package. I propose that the installer be modified to use the following algorithm:

  1. Check whether npm-shrinkrwrap.json has a shasum for the package and the shasum is in the cache.
    1. If so, install from that package and stop.
    2. If not, and the resolved URL is reachable, cache the tarball at that URL and check whether it validates to the correct shasum. If so, install from that package and stop.
    3. If not, install the matching semver specifier from the relevant registry and check whether it validates to the correct shasum. If so, install from that package and stop.
    4. If the shasum is missing or none of the above work, log a warning that the shasum could not be satisfied and continue.
  2. Check whether there is a resolved URL and whether it can be cached.
    1. If so, install from that package and stop.
    2. Log a warning that the URL was not reachable and continue.
  3. Check to see if a package matching the cached semver range can be cached.
    1. If so, install from that package and stop.
    2. If not, log an error and ensure that npm exits with a non-zero code.

Because different ops teams have different requirements, it should be configurable whether to log on 1.iv, 2.ii, and 3.ii. It should also be possible to configure warnings as errors (but probably not whether 3.ii is an error – if you can't find anything that matches what's in npm-shrinkwrap.json, that should definitely be counted as a failure). @rvagg and @timoxley would also find it useful to be able to selectively ignore the resolved field, and if we're making the semantics configurable, we probably want to also allow people to ignore shrinkwrap fields. Also, remember that --no-shrinkwrap is always an option, and that should continue to be supported.

To implement this, we'll need to make the following changes:

  • the npm cache will need to support querying by shasum (part of the cache rewrite I've been working on seemingly forever)
  • npm shrinkwrap will need to cache the shasum of the packages along with the semver range and URL
  • it would be excellent to be able to query the registry for packages by shasum, which the new relational registry should be able to handle relatively straightforwardly (but which isn't there today)
  • the rest of the npm's tooling should be updated to make working with shrinkwrap files less painful (i.e. transparent upgrading of the shrinkrwarp file for npm install -S / npm update -S, etc). This is part of a much larger bikeshed (and as such is beyond the scope of this issue), but if we really want people to be able to rely upon shrinkwrapping, getting the tooling right is an important piece, so it's worth acknowledging.

Could we agree the official NPM registry should not allow packages uploaded with npm-shrinkwrap.json files containing full URLs?

It makes sense to publish deployable applications to npm, such as Cordova, Phonegap, or ember-cli, to the registry with a shrinkwrap file. Also, this is a primary use case for some enterprise ops teams using npm as part of their build and deploy process. Until and unless we come up with a way to distinguish between libraries and applications on the registry, it doesn't make sense to disallow the publishing of shrinkwrapped packages, nor to make any assumptions about which fields in the shrinkwrap files are or aren't useful.

/cc @isaacs, @seldo, @iarna

@Raynos
Contributor
Raynos commented Oct 21, 2014

@othiym23

it doesn't make sense to disallow the publishing of shrinkwrapped packages

If I'm working on an app and run npm i --save X and X or any dependencies of X have an npm-shrinkwrap.json, could it be possible to warn about this ?

Further more, can there be an opt-in npm config flag that I can npm config set 'shrinkwrap.throw' 'true' to have that warning turn into an error and fail npm install ?

I think this could make it easier to avoid this process without actually messing with the registry itself.

If you npm install an existing application as a dev dependency or as a global dependency then this warning should not happen as those are the cordova, phonegap & ember-cli use cases.

@aredridel
Contributor

"shrinkwarp", by the way, is priceless.

@aredridel
Contributor

I don't see any obvious problems here. I'd tend to want to just throw early if there's an shasum mismatch, but perhaps that's paranoia speaking.

@iarna
Member
iarna commented Oct 21, 2014

@Raynos My feeling is that npm i --save X when a shrinkwrap exists should update the shrinkwrap. Does that seem too magical to you?

@othiym23
Contributor

@Raynos

If I'm working on an app and run npm i --save X and X or any dependencies of X have an npm-shrinkwrap.json, could it be possible to warn about this?

No for direct dependencies, yes for transitive dependencies. As @iarna mentions, the correct thing to do is to capture explicit user intent whenever possible, so npm i -[SD] should update npm-shrinkwrap.json in place, instead of requiring you to regenerate the files.

Furthermore, can there be an opt-in npm config flag that I can npm config set 'shrinkwrap.throw' 'true' to have that warning turn into an error and fail npm install?

That's a reasonable request. Sure! (See below.)

If you npm install an existing application as a dev dependency or as a global dependency then this warning should not happen as those are the cordova, phonegap & ember-cli use cases.

An excellent point. We already draw a distinction between top-level dependencies and transitive dependencies in other areas, and it makes sense to do so here as well. It might be that the best thing to do with shrinkwrapped transitive dependencies is to ignore them, and the feedback we'd get from adding this warning / error would be useful in figuring that out.

@aredridel

I'd tend to want to just throw early if there's an shasum mismatch, but perhaps that's paranoia speaking.

Absolutely. This is the configurability I was talking about. Some build teams are going to want missing or not found shasums or URLs to be an error, and some are going to want shasums and / or URLs to be disabled altogether.

@Raynos
Contributor
Raynos commented Oct 21, 2014

@iarna @othiym23

Having npm i X --save update both package.json and npm-shrinkwrap.json would be ideal.

The fact that we have to manually sync node_modules in npm-shrinkwrap.json is tedious, the fact that they can get out of sync is sad too.

The only remaining problem is hand editing package.json which can cause npm install to not reflect the package.json because npm-shrinkwrap.json has a different opinion.

@othiym23
Contributor

The fact that we have to manually sync node_modules in npm-shrinkwrap.json is tedious, the fact that they can get out of sync is sad too.

There's no question that the status quo sucks. The more we lean on npm shrinkwrap as a best practice for deploys, the more important it is that we reduce the friction here.

The only remaining problem is hand editing package.json which can cause npm install to not reflect the package.json because npm-shrinkwrap.json has a different opinion.

In my opinion, inconsistencies between package.json and npm-shrinkwrap.json should raise an early error. You have to be able to trust deploys.

@othiym23 othiym23 added the next-major label Oct 21, 2014
@Raynos
Contributor
Raynos commented Oct 21, 2014

@othiym23

Actually if you run npm install {no args} and it notices that package.json & top level npm-shrinkwrap.json do not agree then it can prompt you to install any modules in package.json that do not agree with npm-shrinkwrap.json.

This assumes that the majority of differences are caused by hand edited package.json ( not hand editing npm-shrinkwrap.json ).

If we have the default --save and the update npm-shrinkwrap.json on save semantics then this will just do the "correct thing"

@othiym23
Contributor

I don't like that behavior, because it can lead to subtly inconsistent behavior (i.e. do surprising or incorrect things). We should absolutely make it easier to keep package.json and npm-shrinkwrap.json in sync, but with that, we should make it more restrictive when dealing with inconsistencies between the two.

@grncdr
Contributor
grncdr commented Oct 21, 2014

The proposed process sounds sensible to me, but like @aredridel I'd much prefer shasum mismatches to be errors by default. Maybe I misunderstood, but the reason for not worrying appears to be:

private registry and enterprise use cases where the ops team republishes dependencies as a matter of course.

This seems like an odd way to do things (republishing an id@version pair). IMO npm should gently discourage it by asking those users to update their config, rather than have a less safe default for everybody else.

@othiym23
Contributor

@grncdr I would prefer to not make npm users pay for the sins of npm publishers, but I could be persuaded. The npm registry already disallow republishing, and it is a non-overridable error if a package's shasum doesn't match the value in its registry metadata. Warnings printed at the end of the install output ought to be prominent enough in most use cases.

@glenjamin
Contributor

πŸ‘ for shasums in shrinkwrap files
πŸ‘ for being able to exclude/remove URLs from shrinkwrap files (CI is firewalled & has caching proxy)

I also would like to add a πŸ‘ for having shasum mismatches error by default. Continuous integration is not generally going to shout loud enough when receiving warnings - if i've shrinkwrapped a particular package and someone's changed it - i'd like to know.

On a mostly related note, is npm ls the proper way to validate that npm-shrinkwrap.json, node_modules and package.json are in sync?

@ehd
ehd commented Oct 21, 2014

I would prefer to not make npm users pay for the sins of npm publishers, but I could be persuaded.

Ultimately npm users are going to pay when publishers made mistakes in republishing an id@version pair. I would rather discourage republishing and reduce potential for human error by changing the default to exiting with an error.

@othiym23
Contributor

It sounds like there's a consensus that shasum failures should be errors rather than warnings. Here's the revised version of the algorithm:

  1. Check whether npm-shrinkrwrap.json has a shasum for the package and the shasum is in the cache.
    1. If so, install from that package and stop.
    2. If not, and the resolved URL is reachable, cache the tarball at that URL and check whether it validates to the correct shasum. If so, install from that package and stop.
    3. If not, install the matching semver specifier from the relevant registry and check whether it validates to the correct shasum. If so, install from that package and stop.
    4. If the shasum is missing or none of the above work, log an error and ensure that npm exits with a non-zero code.
  2. Check whether there is a resolved URL and whether it can be cached.
    1. If so, install from that package and stop.
    2. Log a warning that the URL was not reachable and continue.
  3. Check to see if a package matching the cached semver range can be cached.
    1. If so, install from that package and stop.
    2. If not, log an error and ensure that npm exits with a non-zero code.

Again, it should be able to toggle whether the above are warnings or errors; we're just talking about defaults here.

@glenjamin
Contributor

It might make for a smoother transition if missing shasum is a warning, but mismatch is an error?

@othiym23
Contributor

@glenjamin Maybe, but it also makes it harder to understand what's going on. If you look at the steps of the algorithm, it tries pretty hard to find a version of the package that has a matching shasum, and I think the point that people have made about builds failing loudly and early is pertinent.

@peterlynch

Thank you for furthering the discussion around shasums - this is heading in a better direction.

If not, and the resolved URL is reachable, cache the tarball at that URL

'reachable' in networking terms is prone to problems behind a firewall. Suppose every time npm tries to resolve an external URL, the npm client waits xx seconds for a connect/read timeout. Builds become incredibly slow and annoying. Surprisingly this is common in a private org. If the end user always knows what specific defacto registry URL is always reachable and all others are not, this algorithm seems to give no way to accommodate that other than waiting for a timeout per package with a external resolved URL. This gets back to giving client full control of where to get things.

If not, install the matching semver specifier from the relevant registry

Please define "relevant registry".

@hueniverse

My use case is pretty simple - as a framework maintainer I want to dictate every single version of every module used by the framework, all the way down. The reality is that people do a shit job setting semver, and on top of that, semver is highly subjective. A patch in one module can cause a breaking change downstream.

My goal is to be able to publish a version of hapi that is locked all the way, because no one but me has the ability to really tell what an upstream change mean. I can lock versions of my dependencies, but they are still likely to use wildcard dependencies for theirs. We have also seen the framework stop working due to a patch somewhere and that's impossible for most users to figure out, while their production deployment is now blocked. And on top of that, I can't vouch of the security of a deployment based on a hapi version number, because an attacker can inject an exploit into a deep dependency imported with liberal version requirements.

I don't have much to add to this technical discussion, other then to state that those who object to library publishers using shrinkwrap have the burden of proposing a better solution to a real problem.

IOW - I want a solution that gets me the same result as manually copying/pasting every dependency source file into my code so that it is considered locked.

@Raynos
Contributor
Raynos commented Oct 23, 2014

@hueniverse @othiym23

You can achieve the hapi use case today without messing with the npm mirror or private registry use case by doing the following:

  • run npm shrinkwrap on hapi
  • ensure that ALL dependencies in npm-shrinkwrap.json are npm registry dependencies, none are http tarball links, none are git links
  • by any means, remove all "resolved" fields from the npm-shrinkwrap.json artifact
  • publish it.

It's known that only for npm dependencies just having a version field is sufficient.

When you have a npm-shrinkwrap.json without resolved fields you can run npm install or npm install --registry=my-registry and it will work as expected.

@hueniverse to deal with enterprise customers I recommend adding a step to your publishing strategy that prunes all resolved fields from the npm-shrinkwrap.json artifact. If hapi has git or tarball dependencies somewhere in the tree then your kind of :(

@hueniverse

@Raynos WFM. We don't have any such dependencies. In fact, we only have 2 non-hapi.js dependencies in the framework.

@dougwilson
Contributor

In fact, we only have 2 non-hapi.js dependencies in the framework.

@hueniverse I feel blessed I guess :)

@donaldpipowitch

We have the following that for some developers npm shrinkwrap --dev produces commitish git hashes and for sometime the sha1 id. E.g. calling npm shrinkwrap --dev results in either

"from": "esprima@git+https://github.com/ariya/esprima.git#harmony",

or

"from": "esprima@git+https://github.com/ariya/esprima.git#85fc2f4b6ad109a86d80d9821f52b5b38d0105c0",

Tested both on npm@2.1.7, Git@2.0.1 and Mac OS X 10.10.

@donaldpipowitch

Well we basically use your solution, just with a more complex match: module.resolved.match(/^git(?:(\+ssh|\+https|\+http))?:\/\//).
As this module comes from git+https:// these properties are important. (It would be the same problem with just git:// and your script.)

@mgol mgol referenced this issue in jquery/jquery Nov 20, 2014
Closed

Download builder #1691

@myabc myabc added a commit to opf/openproject that referenced this issue Dec 18, 2014
@myabc myabc Clean unnecessary properties in npm-shrinkwrap.json
See npm/npm#3581

See also commit comment: angular/angular.js@e5dd832

Signed-off-by: Alex Coles <alex@alexbcoles.com>
83aebc4
@myabc myabc added a commit to opf/openproject that referenced this issue Dec 18, 2014
@myabc myabc Add script to clean up npm-shrinkwrap.json
Script stolen verbatim from Angular JS project. See:
angular/angular.js@dd3587a

See also previous commit 83aebc4.

This should be fixed upstream in npm: npm/npm#3581

Signed-off-by: Alex Coles <alex@alexbcoles.com>
e116132
@myabc myabc added a commit to opf/openproject that referenced this issue Dec 18, 2014
@myabc myabc Clean unnecessary properties in npm-shrinkwrap.json
Apparently npm shrinkwrap is not idempotent. This allows for clean
diffs.

See npm/npm#3581

See also commit comment: angular/angular.js@e5dd832

Signed-off-by: Alex Coles <alex@alexbcoles.com>
71fed75
@myabc myabc added a commit to opf/openproject that referenced this issue Dec 18, 2014
@myabc myabc Add script to clean up npm-shrinkwrap.json
Script stolen verbatim from Angular JS project. See:
angular/angular.js@dd3587a

See also previous commit 71fed75.

This should be fixed upstream in npm: npm/npm#3581

Signed-off-by: Alex Coles <alex@alexbcoles.com>
cc428bc
@mwiencek mwiencek added a commit to metabrainz/musicbrainz-server that referenced this issue Apr 15, 2015
@mwiencek mwiencek Run `clingwrap npmbegone` a94855d
@donaldpipowitch

We stumbled over the same problems as the angular guys (again) in socketio/engine.io-client#370. These issues seem to be related and they have long discussions about this problem as you can see in their commits.

@iarna iarna removed the next-major label Jul 15, 2015
@othiym23
Contributor

This has been moved to the npm roadmap, which we're using instead of the confusing next-* labels now.

@glenjamin
Contributor

fwiw, we've been using https://www.npmjs.com/package/shonkwrap to handle this for a while now and not really run into any issues.

@rajadain rajadain referenced this issue in WikiWatershed/model-my-watershed Aug 6, 2015
Merged

Don't let shrinkwrap pull from cache. #658

@maxlang maxlang added a commit to maxlang/cockroach that referenced this issue Sep 29, 2015
@maxlang maxlang Add shonkwrap to clean up nom shrinkwrap
Unfortunately, npm shrinkwrap is inconsistent.

npm/npm#3581

Also the npm-shrinkwrap format has changed once more.
e3f37f0
@OliverJAsh OliverJAsh added a commit to guardian/frontend that referenced this issue Oct 22, 2015
@OliverJAsh OliverJAsh Add shrinkwrap cleaner and npm install docs
`npm shrinkwrap` spits out different metadata depending on the npm version, which makes our diffs much too difficult to read properly.

Browsing [the issue](npm/npm#3581) I found [a little script](https://github.com/angular/angular.js/blob/master/scripts/npm/clean-shrinkwrap.js) we can use to tidy up the shrinkwrap and keep it consistent.

I've also tried to document the npm install process.
e6bc8e3
@othiym23
Contributor
othiym23 commented Nov 6, 2015

This thread is long and covers a lot of ground, but I believe the original issue of the opener (that is, resolved fields come and go in npm-shrinkwrap.json) has been addressed within npm@2 and npm@3. There are a bunch of other possible feature requests and inconsistencies mentioned in this thread, and that makes it tough to act on any of them in a coherent way, so I'm going to close this issue, and if you think there's an outstanding issue with shrinkwrap that isn't covered by issues with the shrinkwrap label, please open new issues.

@othiym23 othiym23 closed this Nov 6, 2015
@vkrol
vkrol commented Mar 7, 2016

The behavior from the issue description continues to occur in the npm@3.8.0. This is very annoying.

that is, resolved fields come and go in npm-shrinkwrap.json

@othiym23 please, can you reopen this issue?

@mockdeep

I'm seeing the same issue. Shrinkwrapping on one computer differs from another:

"dependencies": {
  "babel-code-frame": {
    "version": "6.7.2",
      "from": "babel-code-frame@>=6.7.2 <7.0.0"
    }
  },
  ...
}

Becomes:

"dependencies": {
  "babel-code-frame": {
    "version": "6.7.2",
    "from": "babel-code-frame@6.7.2",
    "resolved": "https://registry.npmjs.org/babel-code-frame/-/babel-code-frame-6.7.2.tgz"
  },
  ...
}

The shorter version is definitely preferable, since otherwise it ends up being TMI, and verifying changelogs on updated packages is more of a headache, especially since Github will often not even show the lengthy diff.

@brianpchsu
brianpchsu commented Aug 1, 2016 edited

I also have the same issue with npm @3.10.3. Basically, I just updated my project number in package.json, but somehow the dependencies verion in shrinkwrap.json changed as well.

From:

"concat-stream": {
      "version": "1.4.10",
      "from": "concat-stream@>=1.4.7 <1.5.0",
      "resolved": "https://registry.npmjs.org/concat-stream/-/concat-stream-1.4.10.tgz"
    },
    "connect-multiparty": {
      "version": "2.0.0",
      "from": "connect-multiparty@>=2.0.0 <3.0.0",
   ...
    },

To:

"concat-stream": {
      "version": "1.4.10",
      "from": "concat-stream@>=1.4.7 <1.5.0",
      "resolved": "https://registry.npmjs.org/concat-stream/-/concat-stream-1.4.10.tgz"
    },
    "connect-multiparty": {
      "version": "2.0.0",
      "from": "connect-multiparty@latest",
    },
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment