Closed rexxars closed 6 years ago
Updated to reduce dependencies on request from @mgcrea
Couldn't wait for having this support. Do you mean this will make ied capable to install modules with git URL like "git+ssh://xxxxxx" from our company's private git lab?
When will you merge and release the new version of ied? I just tested today and having this error:
/usr/local/lib/node_modules/ied/node_modules/rxjs/Observer.js:5
error: function (err) { throw err; },
^
AssertionError: error status code https://registry.npmjs.org/protobuf/git+ssh://git@git.xxxx.com:12888/service-common/protobuf.git#2.0.3: undefined
at checkStatus (/usr/local/lib/node_modules/ied/lib/registry.js:97:20)
at SafeSubscriber._next (/usr/local/lib/node_modules/ied/lib/registry.js:134:12)
This looks great!
I'm wondering: Should we really use registry-auth-token
? It looks like we're only interested in those 3 lines:
var rc = require('rc')('npm', {registry: 'https://registry.npmjs.org/'})
var url = rc[scope + ':registry'] || rc.registry
return url.slice(-1) === '/' ? url : url + '/'
Maybe it would make sense to just inline them...
Concerning the test: A simple unit test would be perfect (maybe stub out fs.readFile
- I know it's ugly, but it's better than no tests at all).
Actually, registry-auth-token
contains code to resolve both the registry url and the auth token for it. The token logic is slightly more complicated. If npm decides to change the way their tokens or registry info is saved, we'll have to update ied
, instead of just updating the dependency. Your call, but the module is pretty slim as it is.
I'll see about adding some tests.
Well, you did ask for tests ;-)
Unfortunately, I found very little tests for the install command and registry parts, so I had to dig a little deep and try to figure out which way to do it. I settled on mocking the http calls using nock and stubbing the registry auth token parts using proxyquire.
I could have mocked out the fs
module as you suggested, but then I'd basically be testing both the registry-auth-token
and rc
modules, which seems wrong for unit tests.
Anyway, I'm heading of to Italy tomorrow for some vacation time, so if you want me to do any changes you'll have to wait for a while or do them yourself ;-)
Happy summer! :sunny: :sunglasses:
Awesome, thanks! Great stuff. Will add this to the next release. Have fun in Italy!
Back from Italy, and rebased! Anything you want me to change here before this can be merged? The tests are a bit flakey, but pass locally - I suspect it's related to the npm registry giving a whole lot of errors lately.
Hey @rexxars ! Cool cool! Thanks for your PR!
I want to make sure the tests are stable before merging this.
(Was thinking of using replay
to mock out the registry later on.)
For now, let's try to get this working. I'm rerunning the tests on CI right now.
If this is what I think it is this would be extremely nice to have this merged in and available. Need any help with getting this finished/finalized?
RIP ied
Checklist
Affected core subsystem(s)
Description of change
By utilizing registry-auth-token, this change ensures that ied resolves which registry to use for a given scope in the same manner as NPM does, and fetches any associated auth token for the registry.
By using the correct registry URL and sending the bearer token along, this allows ied to install private packages from both the public npm registry and private registries.
We're currently using Sinopia at our company, which unfortunately doesn't support the same
/<packageName>/<versionSelector>
endpoint, so I had to add some logic to resolve the correct version from the full package details endpoint (/<packageName>
). Not super happy with the cleanliness of the "legacy mode" as I called it, so feel free to change it however you feel.I could possibly add some tests, but it's quite hard to mock because of how the
rc
module caches its results, along with how it resolves.npmrc
files. I had to resort to some ugly hacks in registry-auth-token.Feedback? This should address parts of #7 and possibly #122.