jspm / npm

NPM Location Service
19 stars 34 forks source link

use correct registry for a given scope #95

Closed gustavnikolaj closed 8 years ago

gustavnikolaj commented 8 years ago

See #94

guybedford commented 8 years ago

This is excellent. I don't have a private setup to test on here, but will gladly merge the PR on faith.

gustavnikolaj commented 8 years ago

I have tested it locally - in fact, I've been using it since I submitted the pull request, by manually linking it in :-) I had a dependency on a module that I couldn't publish to the public registry. I'm in good faith about this, but on the other hand, my experience with jspm is rather limited, so I don't know what the scope of my change is.

Is the code that I have touched being called by any thing external to this module directly?

I think that you'd be able to test it, by trying to install a scoped package available in the public registry, and defining another registry for that scope. I have a package called @gustavnikolaj/migrate (https://www.npmjs.com/package/@gustavnikolaj/migrate) which is a fork of the migrate package with an unmerged PR of mine. If you put the following in .npmrc:

registry=https://registry.npmjs.org
@gustavnikolaj:registry=http://registry.npmjs.org

I think you'd be able to verify that the @gustavnikolaj/migrate package was being fetched through http and not https.

If the code paths I've changed is not being called directly from other modules, I'm confident in what I did.

gustavnikolaj commented 8 years ago

Oh, by the way. I fixed that embarrassing left-over console.log. I can squash the commits and force push the branch if you prefer :-)

guybedford commented 8 years ago

Great, thanks. I've actually been running the 0.17 development on master, and have just forced the branch back to parity with the right branch to separate out that PR. Do you think you could look at rebasing for merge?

gustavnikolaj commented 8 years ago

Sure, gimme a few minutes :-)

gustavnikolaj commented 8 years ago

I think it's in order now :-) Had to do a force push any way, so I merged the commit removing the console.log call.

guybedford commented 8 years ago

Thanks for the quick updates!

gustavnikolaj commented 8 years ago

I verified that it still works.