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

[npm node] support for custom npm registry #1277

Merged
merged 10 commits into from
Nov 21, 2017

Conversation

iamstarkov
Copy link
Contributor

@iamstarkov iamstarkov commented Nov 18, 2017

there are two types of custom npm registries:

affected endpoints:

  • npm node version integration now supports /npm/https/registry.npm.taobao.org/v/express.svg:
    -/^\/npm\/v\/(?:@([^/]+))?\/?([^/]*)\/?([^/]*)\.(svg|png|gif|jpg|json)$/
    +/^\/npm(?:\/(http(?:s)?)\/([^/]+))?\/v\/(?:@([^/]+))?\/?([^/]*)\/?([^/]*)\.(svg|png|gif|jpg|json)$/
  • npm license integration now supports /npm/https/registry.npm.taobao.org/l/express.svg:
    -/^\/npm\/l\/(?:@([^/]+)\/)?([^/]+)\.(svg|png|gif|jpg|json)$/
    +/^\/npm(?:\/(http(?:s)?)\/([^/]+))?\/l\/(?:@([^/]+)\/)?([^/]+)\.(svg|png|gif|jpg|json)$/
  • npm node version integration now supports /node/https/registry.npm.taobao.org/v/express.svg:
    -/^\/npm\/l\/(?:@([^/]+)\/)?([^/]+)\.(svg|png|gif|jpg|json)$/
    +/^\/npm(?:\/(http(?:s)?)\/([^/]+))?\/l\/(?:@([^/]+)\/)?([^/]+)\.(svg|png|gif|jpg|json)$/

downloads related endpoints are not affected due to custom https://api.npmjs.org/ server, which complicates stuff. But apart from complexity, there are several reasons not to modify these endpoints:

  • for public registries its (maybe) okay to use default registry
  • for private registries this downloads metric is (maybe) unnecessary

P.S. inspiration for /npm(/:protocol/:host)/… is jenkins endpoint

extend routes to optionally match protocol (http or https) and host.
utilise ./lib/npm-provider::getNpmRegistryUrl to construct custom
npm registry urls or fallback to standard one.
@paulmelnikow
Copy link
Member

Thanks for this!

I appreciate that you're modeling it the Jenkins badge. Since #1186 it's possible to pass arbitrary parameters through the query string. I'd like to use this for exception / rarely used options, long options, or options which require munging to fit into the URL. This seems like a good place for it. Basically, something like ?registry_uri=.

The benefit is a more consistent and understandable URL pattern. The resultant URL isn't quite as pretty and takes a bit of effort to encode, but it's also not going to be used by many people, and after #701 it should be really easy to encode one of these through the UI.

You can see a few examples in server.js where an array of query string field names is passed in. This is from the dynamic JSON badge:

queryParams: ['uri', 'query', 'prefix', 'suffix'],

},
{
title: 'custom registry :: npm (scoped with tag)',
previewUri: '/npm/https/registry.npm.taobao.org/v/@cycle/core/canary.svg',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about choosing one of these four and removing the others? The title should be something like npm (scoped with tag, custom registry). Same with the node badges. I feel like people will understand the pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screenshot-2017-11-19 shields io quality metadata badges for open source projects 2
screenshot-2017-11-19 shields io quality metadata badges for open source projects 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this now, hope its okay

@@ -36,6 +36,15 @@ function mapNpmDownloads(camp, urlComponent, apiUriComponent) {
}));
}

// npm registry url
function getNpmRegistryUrl(maybeProtocol, maybeHost) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the query string should make this a little simpler. If you still need this function it should go in a new file npm-badge-helpers.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between npm-provider and npm-badge-helpers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the badge providers are in server.js, though a few, like npm, are pulled out. We use -provider to refer to the function which implements the badge.

The -badge-helpers are service-specific utility functions which are invoked by the providers.

Yea, in a couple cases the line is gray. Though this one seems to be a utility function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i got rid off of helper function, now i only use default npm registry uri as a constant to share

server.js Outdated
} else {
// e.g. https://registry.npmjs.org/@cedx%2Fgulp-david
// because https://registry.npmjs.org/@cedx%2Fgulp-david/latest does not work
const path = encodeURIComponent(`${scope}/${packageName}`);
apiUrl = `https://registry.npmjs.org/@${path}`;
apiUrl = `${registryUrl}/@${path}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I wrote tests for the current Node and NPM badges. Could you add tests of the new routes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Nov 18, 2017
@iamstarkov
Copy link
Contributor Author

iamstarkov commented Nov 18, 2017

no problems. i'll change stuff. just to be on the same page. here this is the list to change/add:

  • remove extra examples
  • move helper to npm-badge-helpers.js
  • query params instead of complicating the url pattern
  • tests

if im not right, pls correct me

@iamstarkov
Copy link
Contributor Author

and thanks for the feedback

@paulmelnikow
Copy link
Member

Yup, that's it. And if you keep the helper function, please move it to a different file.

Thanks!

@iamstarkov
Copy link
Contributor Author

only tests left

@iamstarkov
Copy link
Contributor Author

done

@iamstarkov
Copy link
Contributor Author

because of added queryParams and following added indentation to handler diff looks messy.

to make things simpler to review use diff url without whitespace diff https://github.com/badges/shields/pull/1277/files?w=1

@iamstarkov
Copy link
Contributor Author

iamstarkov commented Nov 19, 2017

tests are ok, its just ping to chinese registry is very long/big and mocha is timing out.

what do you think?

@paulmelnikow paulmelnikow changed the title [npm] support for custom npm registry [npm node] support for custom npm registry Nov 19, 2017
@paulmelnikow
Copy link
Member

Excellent! Thanks for the diff; it's easy enough to read. This looks great.

Only one thing I'd ask for, is tests of the license badge. It seems think we have any now. The reason it seems important is that all that code was in flight. You could just pick a package and hard-code the expected value.

Is there another mirror that's faster to reach from the U.S.? Alternatively we could just leave the flaky test. I've solved the timeout issue in IcedFrisby; it's waiting to be shipped in 2.0.

@iamstarkov
Copy link
Contributor Author

sure, i'll add tests for license service

@iamstarkov
Copy link
Contributor Author

about registries

@iamstarkov
Copy link
Contributor Author

its complicated. there used to be some semi-supported mirrors. Nowadays its only registry.npm.taobao.org and r.cnpmjs.org with average ping 275ms and 425ms respectively. So if taobao isnt a good fit, then r.cnpmjs isnt either.

@iamstarkov
Copy link
Contributor Author

~/projects/oss/shields feat/custom-npm-registry
☯ npm i -g nrm -dddddd
npm info it worked if it ends with ok
npm info using npm@5.5.1
npm info using node@v8.9.1
npm http fetch GET 304 https://registry.npmjs.org/nrm 938ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/async 96ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/ini 125ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/node-echo 125ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/extend 129ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/request 146ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/npm 228ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/only 254ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/open 784ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/commander 797ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/mkdirp 60ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/coffee-script 139ms (from cache)
npm http fetch GET 304 https://registry.npmjs.org/jistype 990ms (from cache)

from this log default registry is registry.npmjs.org. but there is as well registry.npmjs.org which is basically a dns alias to default one:

☯ ping registry.npmjs.org -c 11
PING a.sni.fastly.net (151.101.84.162): 56 data bytes
avg 48ms

☯ ping registry.npmjs.com -c 11
PING a.sni.fastly.net (151.101.84.162): 56 data bytes
avg 11 ms

@iamstarkov
Copy link
Contributor Author

i'll use registry.npmjs.com, because its not default one, but fast

@iamstarkov
Copy link
Contributor Author

yay, registry.npmjs.com fixed it

@iamstarkov
Copy link
Contributor Author

done

@iamstarkov
Copy link
Contributor Author

iamstarkov commented Nov 19, 2017

if nothing left to fix, this pull-request should be fine. @paulmelnikow thanks for your input, feedback and patience

@paulmelnikow paulmelnikow merged commit e588bef into badges:master Nov 21, 2017
@paulmelnikow
Copy link
Member

Thanks for all your work!

@iamstarkov iamstarkov deleted the feat/custom-npm-registry branch November 21, 2017 10:10
@iamstarkov
Copy link
Contributor Author

hurray

@iamstarkov
Copy link
Contributor Author

thanks

@iamstarkov
Copy link
Contributor Author

will you release it to https://shields.io/ anytime soon?

@paulmelnikow
Copy link
Member

I can't; I don't have access. Deploys usually happen every 1–3 weeks. Thaddée, who has limited time on this project, is the only sysadmin. He's working on giving me access to deploy and logs, but doing so is complicated because the hosting account (and maybe the servers too) are shared with other services he runs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants