Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert "prune: Fix bug where prune --production would remove dev deps… #166

Closed
wants to merge 1 commit into from

Conversation

bcomnes
Copy link

@bcomnes bcomnes commented Feb 20, 2019

… from the lock file"

This change broke the ability to (easily) create a shrinkwrap file that does not include devDeps. The current behavior of a package published with a shrinkwrap file that includes devDeps is that the devDeps get installed when the package is installed globally (not what you would typically want ever). The only way around this was to prune --prod during prepack to generate a shrinkwrap without devDeps and publish with that (prior to this change going out). See people here discussing that workaround: (here here)

After this change, prune --prod no longer modifies the shrinkwrap file to remove devDeps. Reverting this commit restores the old behavior.

The situation is already looking like a hack but here are two alternative ideas:

  • Perhaps the commit in question IS correct, but a shrinkwrapped package should not install devDeps when installed as a dependency or global package (this seems the most correct and implied behavior described in https://docs.npmjs.com/files/shrinkwrap.json). This is not the current behavior now though.
  • Perhaps running npm shrinkwrap --prod should remove the devDeps from the lock file (without pruning node_modules so its faster and can be restored by shrink-wrapping without the flag). The goal isn't to prune node_modules, its to get my shrinkwrap file right.

I would propose we revert this change in the meantime so that this avenue isn’t broken, until the primary issue is resolved.

This reverts commit cec5be5.

workaround for those interested

  • During prepack: npm prune --prod && rm npm-shrinkwrap.json && npm shrinkwrap Generates the shrinkwrap without devDeps
  • During postpack: git checkout -- npm-shrinkwrap.json && npm i restores devDeps and reinstalls what was removed during the prune.

… from the lock file"

This breaks the ability to (easily) create a shrinkwrap file that does not include devDeps.  The current behavior of a package published with a shrinkwrap file that includes devDeps is that the devDeps get installed when the package is installed globally.  The only way around this was to `prune --prod` during prepack to generate a shrinkwrap without devDeps and publish with that. 

After this change, `prune --prod` no longer modifies the shrinkwrap file to remove devDeps.  

The situation is already looking like a hack but:

- Perhaps a shrink wrapped package should not install devDeps when installed as a dependency or global package (this seems the most correct and implied behavior described in https://docs.npmjs.com/files/shrinkwrap.json)
- Perhaps running `npm shrinkwrap --prod` should remove the devDeps from the lock file (without pruning node_modules so its faster and can be restored by shrink-wrapping without the flag).  Easier than above, but still possibly in hacksvill. 

I would propose we revert this change so that this avenue isn’t broken, until the primary issue is resolved.

This reverts commit cec5be5.
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for taking the time to put this together. After some discussion, it was decided that this isn't really the fix we would be looking for -- I agree that it's definitely a bug for devDeps to be installed when there's a shrinkwrap in a global context, but I'd rather that specific bug gets fixed. Having devDeps in shrinkwrap is by design, since the shrinkwrap file is intended to be a complete description of the tree and we don't currently have plans to allow partial shrinkwraps (the way that used to be possible).

As such, we're gonna pass on this PR, but I hope you continue contributing in the future! (such as contributing a PR to fix that bug). The care is appreciated! ❤️

@zkat zkat closed this Mar 18, 2019
@bcomnes
Copy link
Author

bcomnes commented Mar 18, 2019

I agree that it's definitely a bug for devDeps to be installed when there's a shrinkwrap in a global context

Do you have any advice on where to start a fix for that?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants