feat(shrinkwrap): support private/scoped packages

[5id]

Hey thanks for an awesome package:

This PR adds support for scoped / private npm packages.

Just for background: I was trying to use shrinkpack to solve issues with containerising my Node deployments without needing to include an authentication token in the image or manually copying over the node_modules folder (which totally isn't safe).

Currently if we try to shrinkpack dependencies that are on a private registry, shrinkwrap will fail to add them to the cache with a 401. This PR just adds the --scope=[scope] flag to the npm cache add command so npm will include the credentials in it's request. Surprising that this wasn't already the case.

I wasn't sure how we could add a test into place for this, as we'd almost need a private registry to test this against. Since you're now using docker, we could potentially add https://www.npmjs.com/package/sinopia and test against that as well.

Look forward to your feedback

[JamieMason]

Brilliant @5id.

I wasn't sure how we could add a test into place for this, as we'd almost need a private registry to test this against. Since you're now using docker, we could potentially add https://www.npmjs.com/package/sinopia and test against that as well.

Funnily enough, I've been planning to learn sinopia actually, in order to speed up the dockerised tests which at the moment hit the real npm registry and over time will start to really drag. If you can help me get started with that, it'd be a real help.

This PR just adds the --scope=[scope] flag to the npm cache add command so npm will include the credentials in it's request. Surprising that this wasn't already the case.

I've never (knowingly) used scoped packages before so this has been a bit of a blind spot. Thanks for adding this in. I need to read up some more about them really to help take this forward.

[5id]

@JamieMason I've never used Sinopia but I'm happy to investigate this over the weekend and let you know how I go.

Yeah it's a bit of an uncommon case, used for private/internal npm registrys - feel free to reach out if you have any questions but hopefully I can add some tests to provide some confidence

[JamieMason]

That would be a massive help @5id, thanks.

[5id]

@JamieMason I managed to get sinopia working to proxy the requests through to npm - which will then cache them and serve them locally. It does add a dependency for docker-compose, but I think most people would have that if they already have docker.

Performance wise, I didn't see that much of an increase. I imagine on a slower connection it's more noticeable - perchance there was a config setting that I overlooked. I did see this https://github.com/rlidwka/sinopia/issues/408 - which suggests that sinopia hasn't been actively developed for a while :(

Ideally it would work offline as well but some of the packages download git repositories so we'd still need an internet connection to let them download properly.

I haven't added tests for authentication yet, I wanted to get your feedback early. Are you happy to move forward with this, or should we look for other ways to solve our problem? I already have a plan about how we test that authentication works that won't take very long, but better solutions are king

[JamieMason]

Thanks @5id. I'll take a look myself as soon as I can. I'm surprised as I would've expected a big reduction in install time, like you say, I wonder if everything is set up correctly. I'll pick this up with you as soon as I can.

[5id]

Updated the tests to now test the private repository case. I also found a regression where if shrinkpack failed to complete (e.g. it errored out), it would leave the shrinkwrap file untouched and so all the other tests would pass. I now check to see if the file hasn't changed, and raise an error if thats the case. We could add set -e to the top of the file to catch all these unknown error cases, though I wasn't sure if there was a reason you didn't already have that.

[5id]

@JamieMason I found that Sinopia still makes a request out to npm for every lookup request (rather than just locally) to check that the package information is up to date so that could be where 😞 https://github.com/rlidwka/sinopia/blob/3f55fb4c0c6685e8b22796cce7b523bdbfb4019e/lib/storage.js#L284

I added a config option that seemed to (fingerscrossed) speed things up a little bit. Perchance the next step would be parallelising the installations of the control / shrinkwrap

[JamieMason]

Thanks again for this, it's such a huge help having someone help pick these things up.

I've done a brain dump into #52 to tackle the sinopia/docker/test speed stuff, that way we can discuss that side of things there and the private/scoped packages stuff here.

I will likely cherry pick the commits from your branch on private/scoped packages, then tackle the Docker stuff in a separate branch or PR, does that work for you as well?

[5id]

Glad that I can help,

Thats fine by me, just be aware that there won't be any tests on master for the change without the docker stuff, but if your happy with it then :boom: lets get this happening :+1:

[JamieMason]

Thanks for this @5id, all of it was released in shrinkpack@0.13.1 👍

[JamieMason]

Hoping you could help me out @5id, I'm trying to set the tests up on Travis but am running into some trouble with what I think might be the sinopia stuff.

I have a failing build output here https://travis-ci.org/JamieMason/shrinkpack/builds/147702971 – could you take a look for me please?

Thanks.

[JamieMason]

The changes are in https://github.com/JamieMason/shrinkpack/commit/fe6a65291543d5b66b73b72be41a79cf27cca68b

[JamieMason]

It's not Travis actually, I'm having the same issues locally where the sinopia server can't be hit;

npm info attempt registry request try #1 at 3:45:46 PM
npm http request PUT http://secure_registry:4873/-/user/org.couchdb.user:shrinkpack
npm info retry will retry, error on last attempt: Error: getaddrinfo ENOTFOUND secure_registry secure_registry:4873

I'm using these versions of docker;

$ docker -v
Docker version 1.12.0, build 8eab29e

$ docker-compose -v
docker-compose version 1.8.0, build f3628c7

Could it be that something has changed in how the compose file should be written in these newer versions of docker? Any pointers would be a huge help @5id, thanks.

[5id]

@JamieMason it looks like there was a change in the latest Sinopia Dockerfile that we were using. In docker-compose.yml, we should change image: rnbwd/sinopia to be image: rnbwd/sinopia:stable. I'm seeing another test issue arise after fixing this that I'll look in to tomorrow.

For your own future reference, if you run docker-compose up in the package directory, it will start and show you the output for both npm & test containers (which is where I saw the NPM server exiting straight away).