Initial release #1

pvdlg · 2017-11-06T06:48:44Z

Implementation as discussed in semantic-release/semantic-release#484.

@gr2m can you enable Codecov and Greenkeeper for the repo and add GH_TOKEN and NPM_TOKEN to Travis ? Thanks

Fix semantic-release/semantic-release#126

gr2m · 2017-11-08T03:29:45Z

Codecov & Greenkeeper enabled, tokens set

gr2m

this looks all great, just a few comments and a question 👍

gr2m · 2017-11-08T03:35:18Z

lib/get-client-config.js

@@ -0,0 +1,26 @@
+module.exports = config => {
+  // Form https://github.com/npm/npm/blob/master/lib/cache/caching-client.js#L194


maybe add permanent link? https://github.com/npm/npm/blob/d081cc6c8d73f2aa698aab36605377c95e916224/lib/cache/caching-client.js#L194

When you press y on any file page, it will redirect to permalink

Nice, I didn't know that :)

gr2m · 2017-11-08T03:36:10Z

lib/get-client-config.js

+    userAgent: config.get('user-agent'),
+    defaultTag: config.get('tag'),
+    couchToken: config.get('_token'),
+    maxSockets: parseInt(config.get('maxsockets')),


add radix argument to all parseInt statements. I’m surprised the linking doesn’t catch that. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Octal_interpretations_with_no_radix

Yep.

There is an eslint rule for that: https://eslint.org/docs/rules/radix
But it's not enabled in standard: https://github.com/standard/eslint-config-standard/blob/master/eslintrc.json

This is one of the reason I prefer other solution to standard as they enable mostly style rules and disable a lot of really useful rules to detect these type of bug...Anyway.

gr2m · 2017-11-08T03:39:07Z

lib/get-last-release.js

@@ -0,0 +1,40 @@
+const {promisify} = require('util');
+const {resolve} = require('url');


maybe change this to {resolve: urlResolve}. Line 16 is rather confusing with the promisify and then the resolve call, I was looking for a Promise resolve function :)

gr2m · 2017-11-08T03:40:08Z

lib/get-last-release.js

+  const registry = await getRegistry(publishConfig, name);
+
+  try {
+    const data = await promisify(client.get.bind(client))(resolve(registry, name.replace('/', '%2F')), {


this is quite a lot at once to read easily, can you maybe split it up into multiple lines? At least move the value that resolve(...) returns into an own variable for better readability

gr2m · 2017-11-08T03:41:28Z

lib/get-last-release.js

+    const data = await promisify(client.get.bind(client))(resolve(registry, name.replace('/', '%2F')), {
+      auth: NPM_TOKEN ? {token: NPM_TOKEN} : {username: NPM_USERNAME, password: NPM_PASSWORD, email: NPM_EMAIL},
+    });
+    if (data && !data['dist-tags']) {


why do you check for data her? Should this maybe be !data || !data['dist-tags']?

This is something that come from the old plugin. It's to handle a weird case that happen on npm when the call return data but no dist-tags.
Normally when a package doesn't exist it returns a 404.
If I remember well this situation happens when you publish a version with a scope and you unpublish it, the registry return {} instead of a 404.

gr2m · 2017-11-08T03:43:38Z

lib/set-npmrc-auth.js

+  logger.log('Verify authentication for registry %s', registry);
+  const {NPM_TOKEN, NPM_USERNAME, NPM_PASSWORD, NPM_EMAIL} = process.env;
+
+  if (!getAuthToken(registry)) {


maybe return early instead of wrapping the rest of the code into the if statemen?

if (getAuthToken(registry)) { return }

gr2m · 2017-11-08T03:45:14Z

lib/set-npmrc-auth.js

+  if (!getAuthToken(registry)) {
+    if (NPM_USERNAME && NPM_PASSWORD && NPM_EMAIL) {
+      // Using the old auth token format is not considered part of the public API
+      // This might go away anytime (i.e. once we have a better testing strategy)


can we drop support for NPM_USERNAME & NPM_PASSWORD and only support NPM_TOKEN?

No unfortunately....It's because https://github.com/npm/npm-registry-couchapp doesn't accept login with a token. It accept only the legacy auth (username:password in base 64 + email).

I couldn't find a way to make https://github.com/npm/npm-registry-couchapp authentication works like https://www.npmjs.com/ with a token.

gr2m

pvdlg requested a review from gr2m November 6, 2017 06:48

pvdlg force-pushed the initial-release branch 7 times, most recently from 35d7d79 to cd4aa4f Compare November 6, 2017 07:27

pvdlg changed the title ~~Initial release~~ WIP: Initial release Nov 6, 2017

pvdlg force-pushed the initial-release branch 13 times, most recently from 48609fc to fc68d6e Compare November 6, 2017 16:57

pvdlg changed the title ~~WIP: Initial release~~ Initial release Nov 6, 2017

pvdlg force-pushed the initial-release branch 5 times, most recently from 306fb03 to 0259f0a Compare November 7, 2017 18:55

gr2m reviewed Nov 8, 2017

View reviewed changes

pvdlg force-pushed the initial-release branch 3 times, most recently from 1bd51c6 to 7d5ab00 Compare November 8, 2017 05:13

gr2m approved these changes Nov 8, 2017

View reviewed changes

pvdlg force-pushed the initial-release branch 2 times, most recently from 9b318c8 to e65e82b Compare November 9, 2017 18:17

pvdlg mentioned this pull request Nov 15, 2017

Customize release/publish via plugins semantic-release/semantic-release#484

Closed

pvdlg force-pushed the initial-release branch from e65e82b to 577a189 Compare November 16, 2017 18:47

feat: Initial release

d9f3feb

pvdlg force-pushed the initial-release branch from 577a189 to d9f3feb Compare November 17, 2017 03:43

gr2m mentioned this pull request Nov 19, 2017

Artifactory npm auth fails semantic-release/semantic-release#500

Closed

pvdlg merged commit 59a7d00 into master Nov 20, 2017

pvdlg deleted the initial-release branch November 21, 2017 05:01

pvdlg mentioned this pull request Nov 22, 2017

Lint with XO semantic-release/semantic-release#519

Merged

		@@ -0,0 +1,26 @@
		module.exports = config => {
		// Form https://github.com/npm/npm/blob/master/lib/cache/caching-client.js#L194

		@@ -0,0 +1,40 @@
		const {promisify} = require('util');
		const {resolve} = require('url');

Initial release #1

Initial release #1

Uh oh!

Conversation

pvdlg commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gr2m commented Nov 8, 2017

Uh oh!

gr2m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvdlg Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gr2m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pvdlg commented Nov 6, 2017 •

edited

Loading

pvdlg Nov 8, 2017 •

edited

Loading