This is promising, and I understand the use case for your team and think that this is the least disruptive way to handle it, so where is this at?

I should have time to work on this some more in the next few days. Sorry for the delay!

#10265 seems super-helpful for this.

OK, by including @terinjokes's PR (thanks!), this became a whole lot easier, so here it is, including tests. I edited the title and description of this PR to show completion and include the net effects. 👍 💥

Awesome! I'll talk to @iarna about including this in next week's release (it's a little late in the week to be including in tomorrow's, especially because we've been doing a lot of cheese-moving around shrinkwrap over the last few weeks, so things are maybe not quite as solid as we'd like, and we need to review these kinds of patches carefully). Thanks to both of you for putting this together! (We'll also probably land @terinjokes's change first and then rebase this patch on top of that, so plz don't squash any commits until we get a chance to do that.)

Thanks for putting this together. This will allow us to do better CI in our project.

@andersjanmyr : Take a look. P.S., have you found any workarounds in the meantime (other than merging your "dependencies" and "devDependencies" lists?

@avindra We ended up just packaging everything up in a tar-file for production after a clean build
without the shrinkwrap. This has worked fine, since we are using the same architecture for development and for production. We mainly use shrinkwrap to pin down our development dependencies, especially for grunt-plugins.

What is the expected behavior of shrinkwrapping optionalDependencies?

Right now optionalDependencies are included in the shrinkwrap file, and can cause an installation failure when they fail to hydrate. For example, when a shrinkwrap is created on an OS X machine, thus including "fsevents", and an install is then attempted on Linux.

Should we tag optionalDependencies, as this pull request does for devDependencies, and allow them to fail on hydration?

Provided something like isTreeOptional can be made, sure. Sounds like it would solve #2679.

@bengl I've got some free time tomorrow. I'll look into if it can be made.

@terinjokes Any progress on the isTreeOptional stuff? I'm feeling the pain with shrinkwrapped fsevents on a linux host, so I'm willing to help out however I can.

@evocateur not yet, i'll pop it off the queue shortly.

Also, what happens in these cases:

Prerequisites:

• Existing npm-shrinkwrap.json which includes both dev and prod deps

Cases:

• npm install --save-dev new_package
  • Will the new_package be added to the shrinkwrap?
• npm install --save-dev [email protected]
  • Will the existing_package be updated in the shrinkwrap?
• npm install --save [email protected]
  • Will the existing dev deps remain untouched in the shrinkwrap?

@Whoaa512 In all cases, you need to run npm shrinkwrap --dev after the install commands, otherwise there will be no changes to npm-shrinkwrap.json.

What about when --also=dev is added to the aforementioned cases? Looking at the install code and from local testing saveShrinkwrap inside install does not respect this setting, even though the docs/changelog would tell you otherwise.

@terinjokes I've verified that failing optional shrinkwrapped dependencies don't break the install, they just warn. I see this behavior going as far back 3.3.12. (probably further)

Now there have been some more complicated wrinkles around these, so I could believe there's a bug lurking, but a reproducer would be really helpful here!

@iarna here's a contrived example:

package.json:

{
  "optionalDependencies": {
    "test-c": "7.0.0"
  }
}

npm-shrinkwrap.json:

{
  "dependencies": {
    "test-c": {
      "version": "7.0.0",
      "from": "[email protected]",
      "resolved": "https://registry.npmjs.org/test-c/-/test-c-7.0.0.tgz"
    }
  }
}

The optionalDependency is a version that's not on the registry, and so the shrinkwrap file could never really be generated, but I think this properly simulates a version having been unpublished.

• npm install with these two files fails, with fetch timeouts on the non-existent package.
• npm install without the shrinkwrap succeeds, with warning.

Ok, I'm sorry I'm just really digging into the proposed solution now but…

devDependency-only items in the shrinkwrap are marked with dev: true.
npm i --only=prod will not install anything in the shrinkwrap marked dev: true.
npm i --only=dev will only install things in the shrinkwrap marked with dev: true.

Why are we adding anything to the shrinkwrap? Why not do these checks in inflate-shrinkwrap?

@iarna Hmm, it's been a while since I did this, but as I recall, it was my understanding that knowledge of dev-or-not was not available in inflate-shrinkwrap. Is it there in the tree object?

It is! So somewhere in the asyncMap I'd…

https://github.com/npm/npm/blob/master/lib/install/inflate-shrinkwrap.js#L22

Add a check like:
if (tree.package.devDependencies[name]) {

If you're skipping it just return next()

Oh awesome! That simplifies things. I'd still need isTreeDev (and the soon to arrive isTreeOptional) though, right? Otherwise we're only looking at one level.

I was thinking that at first too, but no, because you never add that child to the tree, you never recurse to any of its children, and so there's never any need to check on them.

The only other detail, which is probably an outstanding bug, is:

https://github.com/npm/npm/blob/master/lib/install/deps.js#L110

which should account for only=dev(elopment) and only=prod(uction) too.

Cool I rebased again. This is gonna need some changes when https://github.com/npm/npm/compare/iarna/fix-dev-prod-env is merged.

@bengl thanks for getting this to move forward. let me know if there's anything I can do to help.

fix-dev-prod-env is not a blocker on this (it's a minor structural change and will need tests to land).

@bengl Where is this at here? Do you think this is ready to land?

@iarna It passes the tests I had for it. If you want I can clean up the commits a bit first?

I'm putting this on my list to finish and get landed. (Which it sounds like may not involve any real work so 🎉!)

Ok, the current status of this is:

This adds new attributes to npm-shrinkwrap.json for optional and dev to record what kind of dependency they were saved as.

It makes it so that npm install --only=prod won't install deps that were saved to the shrinkwrap as dev deps.

Currently at adds no new behavior for the optional flagging.

I'm ok with landing this as is and we can add behavior around optional deps later as desired.

SGTM.

I'm just running the tests for the ported and squashed version and if all looks good I'll land it.

Looks good!

Merged to our release-next branch. This will be in the next release of npm (scheduled for September 8th).