Diff does not contain packages if npm-shrinkwrap.json is missing "resolved" fields

[markerikson]

I just had my first package update sequence go badly for me. As usual, I had done an npm install --save-dev (in this case, the "sinon-chai" package), confirmed the package was working as I wanted, and then run npm shrinkwrap --dev followed by shrinkpack. However, despite my package.json having been updated by the install, shrinkpack didn't list any diff'd packages. I went back and forth on this for a while, and finally figured out what was going on.

I already knew that Shrinkpack relies on rewriting the "resolved" field in npm-shrinkpack.json to point to the local files under./node_shrinkwrap. However, it appears that on occasion, npm shrinkwrap does not actually write out a "resolved" field for some packages. In my case, the relevant shrinkwrap entry was:

    "sinon-chai": {
      "version": "2.8.0",
      "from": "sinon-chai@latest"
    }

Because shrinkwrap didn't supply a "resolved" field, Shrinkpack didn't see anything that needed to be included.

I tried a few times to get shrinkwrap to actually write out that field. There were a couple vaguely-related NPM issues, such as npm/npm#3581 , which suggest this is a sorta-known issue with no real obvious solution. One suggestion was to run npm clear cache, which I did but didn't help.

Ultimately, I tried running npm view sinon-chai to see the actual tarball URL, then manually added that as a "resolved" field to npm-shrinkwrap.json. When I reran Shrinkpack, it correctly indicated:

+ [email protected]
shrinkpack +1 -0

Now, obviously Shrinkpack can't do anything to force NPM install to write out npm-shrinkwrap.json properly. However, could it maybe do a little extra work to try to track down a valid resolution URL or something? For example, I note that if I look at my /node_modules/sinon-chai/package.json, it has a "dist" field with the same correct tarball URL. Maybe if npm-shrinkwrap.json doesn't have a "resolved" field for a package, Shrinkpack could look for the "dist" field in the installed package's package.json?

I'm actually looking at Shrinkpack's shrinkwrap.js, and I see that line 58 has if (is.string(object.resolved)), but I don't actually see it using "resolved" anywhere. A bit confused on that.

[JamieMason]

Thanks a lot Mark, that info is really helpful. I'll take a look into it soon.

[markerikson]

A quick followup. I loaded up my current npm-shrinkwrap.json and did a filter on its "dependencies" list. There are 734 total items listed. Of those, 204 do not have a "resolved" property, meaning that 530 are "resolved".

Interestingly, my node_shrinkwrap folder currently has 569 items in it. Close to the number of "resolved" items, but a bit more. Not sure why the difference. I see 735 folders in /node_modules itself.

Is there a specific reason why you're looking for the "resolved" property in npm-shrinkwrap.json? What does it mean if NPM doesn't include that, besides the possibility that it's a bug? Does Shrinkpack actually want to always grab tarballs for literally every entry in npm-shrinkwrap.json, or does it need to only grab tarballs for resolved entries?

[JamieMason]

I think my fix for #19 will also address this one @markerikson.

[JamieMason]

Version 0.5.0 has been released, please could you give it a try and update or close this issue?

I tried installing and shrinkpacking sinon-chai (and sinon and chai peers that I was prompted for) and all seemed to work as expected.

Thanks a lot @markerikson.

[markerikson]

TL;DR: nope, Shrinkpack seems to be behaving the same - it's fine if given valid shrinkwrap input, the question is what should happen if it sees entries that are not "resolved".

Hmm. I updated Shrinkpack globally to 0.5.0 and tried doing npm i --save-dev sinon-chai on a repo that didn't have it yet. NPM installed, package.json got update. Now, interestingly, when I re-ran npm shrinkwrap --dev, this time the "sinon-chai" entry in npm-shrinkwrap.json did actually have a "resolved" field.

However, the general behavior that I'm seeing in Shrinkpack itself is still there. If the "resolved" field exists in a shrinkwrap entry, Shrinkpack correctly detects it and will add it to /node_shrinkwrap if necessary. If the "resolved" field does not exist, Shrinkpack ignores the entry, because it's explicitly looking for that field.

So, this kinda goes back to the last question I was asking: what does it mean to both NPM and Shrinkpack if the "resolved" field does not exist in an entry? Should that field actually exist for every entry in a shrinkwrap file, is NPM being buggy, or does that indicate that a given entry is lesser/not-important/something? If an entry exists in the shrinkwrap file but doesn't have a "resolved" field, should Shrinkpack actually be trying to dig up the package tarball path on its own?

In other words, the first thing I'm trying to understand is what that the proper contents of a shrinkwrap file should be, and then the question is what should Shrinkpack's behavior be in response to that.

[JamieMason]

No fix yet, but I've refactored the code that traverses the dependency graph to try and make my intentions clearer. I was looking for resolved to determine if what I had a reference to at that time was a package, or something else.

I have an idea to do what you suggested using npm view, but could you give me some steps to follow that will give me an entry with a missing resolved field? I had a try with a new project using sinon-chai, but the field was present.

Thanks.

[DylanPiercey]

@JamieMason shy not just check if the tarball exists in the current npmcache folder?

[JamieMason]

That could work — we check every package to see if it is in the cache, then if it isn't we run this. Line 36 uses the resolved property to know where to fetch the tarball, but if that is missing we could use [email protected] instead.

One case where this would not work is packages that are not hosted in npm and are missing the resolved property, but that's a pretty niche edge case that we can kick into the long grass for the time being I think.

/cc @DylanPiercey @markerikson

[markerikson]

For what it's worth, I've got a stripped-down version of my current prototype published as a sample project over at https://github.com/markerikson/react-redux-cesium-testing-demo . You can see my actual list of deps in package.json, and the current output of npm shrinkwrap in npm-shrinkwrap.json. Note that per my earlier comments, a significant portion of the shrinkwrap entries don't have "resolved" fields (such as "ansi-styles" and "anymatch").

You could probably use that project as a testbed.

[JamieMason]

Great, thanks

[JamieMason]

I think this should be fixed in 0.6.0, I actually experienced the same issue while running shrinkpack locally against itself — the espree dependency had a missing resolved property. That's working locally now.

[markerikson]

Just updated my global Shrinkpack to 0.6.0 and running it now. It's clearly doing something different than it was before. I'm seeing a bunch of new libs being shown as added diffs as it's running. Also... it's completely pegging my CPU because it's firing off all these background NPM tasks. It's also taking quite a while to run. Now, as I said, my project had at least a couple hundred non-resolved entries in the shrinkwrap file, but it just feels like it oughta be able to chew through this faster. (Admittedly, one issue for me personally is that my company installs a bunch of security and process monitoring software on our machines, and that software insists on inspecting new processes for validity, pegging my CPU as it does so. Running Git commands also pegs my CPU because of the nested process tree.)

Per my earlier comment, would it be possible to have Shrinkpack check /node_modules/some_installed_package/package.json for the "dist" field, which should contain the tarball path? Seems like that might cut down on some of the need to query NPM for the metadata every time. Fewer child processes and network requests.

Still, definitely a step in the right direction as far as "correct" output. Way slower at the moment, but it does look like it should ultimately wind up with all my transitive deps being packed up, whereas before it was clearly missing a large percentage because NPM wasn't giving enough info.

[JamieMason]

Totally agree @markerikson, performance just hasn't been a priority up to now. Initially I just wanted to prove the idea and see if repointing shrinkwrap at relative file paths was even possible, then navigate the edge cases and kinks so it's all more stable.

With the above now in better shape, performance is now the main priority behind any bugs, should they come up.

Thanks a lot, I'll create issues from some of your points.

[markerikson]

End result:
shrinkpack +248 -0

Took about 20 minutes to run. Fortunately, I just re-ran shrinkwrap+shrinkpack, and the second time around things finished in just a couple seconds (because, presumably, everything was already in cache). So yeah, better performance would be good, but looks like it should mostly be a first-run issue.

[JamieMason]

Yeah, it's definitely too slow. As you say, first-run is the worst hit, after that it's just a case of removing old deps and adding new ones – with everything else being left alone.