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

Freeze dependencies versions #32

Open
kaievns opened this issue Apr 18, 2016 · 11 comments
Open

Freeze dependencies versions #32

kaievns opened this issue Apr 18, 2016 · 11 comments

Comments

@kaievns
Copy link
Contributor

kaievns commented Apr 18, 2016

I've noticed that you have either ^ or completely empty deps versions in package.json, that's kind of bad practice as npm will just install the most recent version on every deployment and it also makes it very difficult to track deps updates.

i'd suggest we use the --save-exact to lock the versions in.

@nwinch
Copy link
Contributor

nwinch commented Apr 18, 2016

We're using shrinkwrap, hence the blank versions.

@kaievns
Copy link
Contributor Author

kaievns commented Apr 18, 2016

oh i see, that's what that thing for. ok, no problem then

@kaievns kaievns closed this as completed Apr 18, 2016
@davidbanham
Copy link
Contributor

Should we actually be using shrinkwrap, though? It's full of bugs that have bitten us multiple times. The two that stick out to me are:

  1. Npm fills your /tmp folder with garbage when there's a shrinkwrap file present until it eventually fills your disk
  2. Npm can't do cache invalidation properly. This coupled with the fact that shrinkwrap directly fetches tarballs means that unpublished packages have weird consequences divorced from their origin by like a year.

I'd rather we rely on semver and a test suite to ensure our deps stay sane in development. The problem of deps staying consistent between development and deployment is now handled by Docker.

The problems of upstream libraries not adhering to semver is, I think, a useful tension. If we can't trust an author to adhere to semver why are we trusting their code?

@kaievns
Copy link
Contributor Author

kaievns commented Apr 18, 2016

i don't know lets discuss this. as far as i know people usually --save-exact. but if this thing does it better than i'm open for giving it a go

@kaievns kaievns reopened this Apr 18, 2016
@davidbanham
Copy link
Contributor

Yep, if we want exact versions I think that'd be a much better mechanism than shrinkwrap.

The question, though, is do we really want to circumvent semver? It's a useful thing.

@nwinch
Copy link
Contributor

nwinch commented Apr 18, 2016

as far as i know people usually --save-exact

As far as I know people usually use shrinkwrap? It exists for this very reason :)

Yep, if we want exact versions I think that'd be a much better mechanism than shrinkwrap.

How are exact versions a better mechanism? It doesn't solve the dependency version issue. Dave could those two issues you pointed out be fixed with npm 3.x? It seems like issue 1 is referenced a few times on NPM's github, and people have various workarounds - could we do the same? Kinda sucks we have to, but seems like we could fix it anyway. For point 2, surely we could clear the cache with a script/snipper when needed?

The problem of deps staying consistent between development and deployment is now handled by Docker.

Interesting. How does Docker solve this? Is there a similar style shrinkwrapped file that Docker uses or something?

@kaievns
Copy link
Contributor Author

kaievns commented Apr 18, 2016

@davidbanham tbh, i wouldn't trust programmers on doing sane things with semver :)

@nwinch
Copy link
Contributor

nwinch commented Apr 20, 2016

@davidbanham Could you please shed light on my responses above?

Also let's resolve this discussion so we can close it off and move on. So far there doesn't seem any compelling reason not to continue using shrinkwrap. Dave you suggested that Docker will do all of that for us, could elaborate some more on that?

@davidbanham
Copy link
Contributor

Oh man this is an ancient issue!

The reference to Docker is about keeping deps the same between CI and production. You take the same container from CI to production, so if your tests pass in CI or UAT, then things will definitely be fine in prod. There's no option for some subsequent install stage to break because Gary broke his API without bumping the major (or just deleted his whole damn package from npm). It does this because the node_modules directory is part of the generated image. They're effectively vendored once the image has been built, but they don't need to be part of your source control. Neat!

The first issue isn't fixed by npm3. That issue is still open and is coming up on its second birthday. We currently do use workarounds, because we have to, but they're shit. The mechanism is that we have a crontab that just does rm -r /tmp/*npm every hour or so. If that happens to run while something is midway through installing, too bad it's ruined now. If something goes haywire and enough installs run that your disk is full in less than an hour, too bad your machine is locked up and your processes are down.

The second issue is actually the cache on npm's CDN rather than anything on the local machine. This is what took down Ordermentum production a few months ago. Some package it was using (shrinkwrapped) had actually been accidentally deleted by the author about a year prior. npm inc. hit the "clear cache" button after the changes they made in the wake of the left-pad fiasco. The URL to that package's tarball started returning 404s and all of a sudden we were unable to bring up new processes and the whole thing went down. On takeaway night. Just as the pizza arrived. Ruined my evening.

@nwinch
Copy link
Contributor

nwinch commented Aug 4, 2016

Oh man, not on pizza night... that's rough. The old girl Ordermentum sure knew when to throw a tantrum didn't she? I miss her.

@nwinch
Copy link
Contributor

nwinch commented Aug 8, 2016

I've removed the shrinkwrap file in #67 as 1) it was done with npm 2, and 2) I didn't see it necessary to add back in due to lack of activity with this project, and the fact that this project will most likely be rewritten in conjunction with redbeard UI.

If the admin does go full Nicolas Cage and appear absolutely everywhere, then I think we should revisit including shrinkwrap for stability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants