Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Remove npm-shrinkwrap #3512

Closed
hueniverse opened this issue Jun 5, 2017 · 19 comments
Closed

Remove npm-shrinkwrap #3512

hueniverse opened this issue Jun 5, 2017 · 19 comments
Assignees
Milestone

Comments

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jun 5, 2017

npm 5 completely broke the solution we were using until now. It forces changes on the existing file so no manual management is possible. The new format then breaks older versions of npm. I am not sure what's the full replacement but for now I'll just lock the package.json versions for the direct dependencies which is not perfect but better than nothing.

If anyone has suggestions, let me know.

@hueniverse hueniverse added this to the 16.4.1 milestone Jun 5, 2017
@hueniverse hueniverse self-assigned this Jun 5, 2017
@hueniverse hueniverse closed this in d249ebb Jun 5, 2017
@devinivy
Copy link
Member

@devinivy devinivy commented Jun 5, 2017

Anyone know more about this? You can't author/manage shrinkwraps manually anymore?

@corbinu
Copy link

@corbinu corbinu commented Jun 5, 2017

@devinivy No the new behavior is designed to be much more like yarns work flow.

Docs are here: https://docs.npmjs.com/files/package-locks

@zkat would be the source of truth for all things npm@5

@zkat
Copy link

@zkat zkat commented Jun 5, 2017

So, a couple of things:

  1. The new format should be backwards-compatible. Is there something that's breaking? D:
  2. we've been moving away from a "pick your own shrinkwrap" model for a while. One of those changes was adding devDeps. Another was no longer allowing partial shrinkwraps. I'm not sure what else would be breaking you here, aside from moving to the new format: the only difference besides the overall format change is that we --save by default, which has been updating shrinkwraps already since npm@3.

@devinivy
Copy link
Member

@devinivy devinivy commented Jun 5, 2017

@zkat thank you very much for the speedy response!

I imagine part of our problem is that we use a partial shrinkwrap (if I remember correctly). But I'm not sure that's necessary for us. I still don't fully understand the issue @hueniverse is keying-in on here. I'd guess that the "forced changes" to the shrinkwrap could be ignored using a git checkout after an npm install.

@zkat
Copy link

@zkat zkat commented Jun 5, 2017

to be clear: unless you're installing new modules or removing them, npm-shrinkwrap.json should not change, unless something weird's going on. In which case we should fix it. /cc @iarna to make sure I got this right.

We really haven't supported partial shrinkwraps for a while, and last time I asked, I thought hapi had stopped doing that (it was one of the reasons we weren't making the change yet!)

@iarna
Copy link

@iarna iarna commented Jun 5, 2017

I concur with everything @zkat said here. I'm pretty sure we patched the partial shrinkwrap out of hapi before the npm@4 launch (I opened in issue here in advance of that, since we knew that change was going to break you all.)

We definitely aren't going to be supporting manually editing your shrinkwraps going forward, but if you want to mutate it in a consistent way the postshrinkwrap lifecycle exists to support that.

But that being said, it's possible that the package-lock.json will meet your needs fine (it's the same file but with a different name).

I'd like to know though: What problem were you all solving with your npm-shrinkwrap.json? We'd like ensure that you can solve it in a satisfactory way.

@iarna
Copy link

@iarna iarna commented Jun 5, 2017

@hueniverse If you'd like, I'd be happy to PR a postshrinkwrap that gives makes all npm's give you shrinkwraps comparable to your old one.

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 5, 2017

My goal is to have full control over every dependency hapi uses all the way down. I also want to be able top block one module (isemail) from being installed. Before npm 5 I was able to do just that with a manually created shrinkwrap file. It was working perfectly with npm 4. I am not picky about the solution as long as it allows me to do what we used to.

The reason, btw, is security. When someone installs my package, I want them to know with 100% certainty that I approved, reviewed, and tested every dependency my module brings with it. Just because some author down the line decided to publish a new version should not pop up on my machine unless I explicitly asked for it.

The normal shrinkwrap process for production code doesn't really prevent developers using my code to understand which deep dependency they should grab and allow in their local deployment and which they should not. I provide a fully tested package.

@zkat @iarna Would this still be possible with the new setup? Thanks for the help!

@zkat
Copy link

@zkat zkat commented Jun 5, 2017

@hueniverse So, one thing that this new shrinkwrap format can do is make it easier to review diffs by minimizing churn. I think what you could try is to just throw away diffs you don't want for transitive dependencies as long as the required semver ranges match. So if you see something you don't like, just don't commit it.

@iarna
Copy link

@iarna iarna commented Jun 6, 2017

@hueniverse

My goal is to have full control over every dependency hapi uses all the way down.

That is exactly what it's for.

I also want to be able top block one module (isemail) from being installed. Before npm 5 I was able to do just that with a manually created shrinkwrap file. It was working perfectly with npm 4. I am not picky about the solution as long as it allows me to do what we used to.

If you remove a module by hand then npm@5 will maintain that. This is a little weird and in any version of npm will result in npm ls reporting missing dependencies, but it will work.

Would this still be possible with the new setup? Thanks for the help!

Yup, there's really nothing special to it.

The only other special thing that you were doing previously was stripping the shrinkwrap down to just be version specifiers (no from, no resolved). Was there a technical reason for this, or was it just that it was easier to maintain by hand?

We don't generate from any more, but we do generate resolved. If you do need resolved removed, that can be arranged.

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 6, 2017

So...

I started with this file: https://raw.githubusercontent.com/hapijs/hapi/b18fa26f3ab839cadecea64c25fab8bf2116b5a8/npm-shrinkwrap.json

I ran npm i

and ended up with this file: https://gist.github.com/hueniverse/033dab4eebadd0a1b981e615ac6a9ef3

Note, I didn't do anything, just ran npm i and it converted the file I had in place.

I then edited this file to remove dependencies I didn't want (basically all the dev ones). Ran npm i again and they were all back in there. I did notice that isemail which is a dependency of joi didn't come back. So it might just be dev deps being forced. Can we fix that?

@iarna
Copy link

@iarna iarna commented Jun 6, 2017

@hueniverse You should maybe go read the commit messages in the PR I put together? I think it'll answer all of your questions here.

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 6, 2017

@iarna did I miss a link?

@iarna
Copy link

@iarna iarna commented Jun 6, 2017

@heuniverse Immediately above your comment, #3513

@iarna
Copy link

@iarna iarna commented Jun 6, 2017

The PR goes step by step from the shrinkwrap you had previously and with each commit makes the smallest change possible, including lifecycle scripts necessary to support that.

The one thing we don't support is skipping dev dependencies. For various technical reasons they need to be in the shrinkwrap, but they won't be installed if you run npm install hapi nor will they be installed if you did npm install --prod in a hapi check out. That is, they follow the same rules for if they're installed as you'd have without one.

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 6, 2017

My bad. Thanks for the commit.

The problem with this is that it is a maintenance nightmare. I edit this file by hand, so now it is huge list and I will need to figure out how to keep the dev deps up to date automatically... This is a lot more work.

Why is it, if I remove a dependency, it doesn't come back, but if I remove a dev dep, it does? If you are going to force dev deps, why do you need to write them back into the existing file?

@iarna
Copy link

@iarna iarna commented Jun 6, 2017

Ok, good news first:

I realized that postshrinkwrap script can trivially strip the dev deps to. The variation at c6a7c3d in conjunction with c723700 is essentially identical to what you had before. The variation at 70c880f is the same but tracks integrity too.

Indifferent news next:

You can update just your dev dependencies with:

npm update --only=dev

And not so great news:

Until npm@5, npm had never actually supported what you're trying to do with supressing isemail. I'm testing with a package has one dependency on hapi@16.4.0:

With npm@2: I get a tree with isemail@2.2.1. It also has the following extraneous deps: code@4.0.0, inert@4.2.0, lab@13.1.0, vision@4.1.1.

With npm@3: I get the same extraneous deps. The tree is initially missing isemail, but subsequent installs have isemail@2.2.1.

With npm@4: I get the same behavior as npm@3.

With npm@5: This is the only version that actually keeps isemail suppressed.

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jun 6, 2017

@hueniverse You can get the old install behaviour with npm i --no-save.

This can be persisted by adding a .npmrc file to the repo with:

save=false

This should enable you to develop, while manually maintaining the shrinkwrap file, as you used to.

@hueniverse hueniverse reopened this Jun 7, 2017
@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 8, 2017

Thanks everyone. I'll use @kanongil suggestions and keep the file for now.

@hueniverse hueniverse closed this Jun 8, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants