From dd2a51ccc51d76149e1f369c97326d32cd483485 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Mon, 29 Jan 2018 10:39:14 -0500 Subject: [PATCH 1/6] feat(publish): Publish command uses publishConfig.access in package.json For npm compatability, `yarn publish` should check `publishConfig.access` in package.json and use it as if the `--access` option was passed. #5279 --- __tests__/__mocks__/npm-registry.js | 10 +++ __tests__/commands/publish.js | 61 +++++++++++++++++++ .../fixtures/publish/minimal/package.json | 4 ++ .../fixtures/publish/public/package.json | 7 +++ src/cli/commands/publish.js | 11 +++- 5 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 __tests__/__mocks__/npm-registry.js create mode 100644 __tests__/commands/publish.js create mode 100644 __tests__/fixtures/publish/minimal/package.json create mode 100644 __tests__/fixtures/publish/public/package.json diff --git a/__tests__/__mocks__/npm-registry.js b/__tests__/__mocks__/npm-registry.js new file mode 100644 index 0000000000..c5f54cb2a7 --- /dev/null +++ b/__tests__/__mocks__/npm-registry.js @@ -0,0 +1,10 @@ +/* @flow */ + +import Registry from '../../src/registries/base-registry.js'; +import type {RegistryRequestOptions} from '../../src/registries/base-registry.js'; + +export default class NpmRegistry extends Registry { + request(pathname: string, opts?: RegistryRequestOptions = {}, packageName: ?string): Promise<*> { + return new Promise(resolve => resolve()); + } +} diff --git a/__tests__/commands/publish.js b/__tests__/commands/publish.js new file mode 100644 index 0000000000..9575c21416 --- /dev/null +++ b/__tests__/commands/publish.js @@ -0,0 +1,61 @@ +/* @flow */ + +import {run as buildRun} from './_helpers.js'; +import {run as publish} from '../../src/cli/commands/publish.js'; +import {ConsoleReporter} from '../../src/reporters/index.js'; + +const path = require('path'); + +const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'publish'); + +const runPublish = buildRun.bind( + null, + ConsoleReporter, + fixturesLoc, + async (args, flags, config, reporter, lockfile, getStdout): Promise => { + // $FlowFixMe + config.registries.npm.request = jest.fn(); + config.registries.npm.request.mockReturnValue( + new Promise(resolve => { + resolve({status: 200}); + }), + ); + await publish(config, reporter, flags, args); + return getStdout(); + }, +); + +test.concurrent('publish should default access to undefined', () => { + return runPublish([], {newVersion: '0.0.1'}, 'minimal', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual(undefined); + }); +}); + +test.concurrent('publish should accept `--access restricted` argument', () => { + return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'minimal', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('restricted'); + }); +}); + +test.concurrent('publish should accept `--access public` argument', () => { + return runPublish([], {newVersion: '0.0.1', access: 'public'}, 'minimal', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('public'); + }); +}); + +test.concurrent('publish should use publishConfig.access in package manifest', () => { + return runPublish([], {newVersion: '0.0.1'}, 'public', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('public'); + }); +}); + +test.concurrent('publish should allow `--access` to override publishConfig.access', () => { + return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'public', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('restricted'); + }); +}); diff --git a/__tests__/fixtures/publish/minimal/package.json b/__tests__/fixtures/publish/minimal/package.json new file mode 100644 index 0000000000..1545befa16 --- /dev/null +++ b/__tests__/fixtures/publish/minimal/package.json @@ -0,0 +1,4 @@ +{ + "name": "test", + "version": "0.0.0" +} diff --git a/__tests__/fixtures/publish/public/package.json b/__tests__/fixtures/publish/public/package.json new file mode 100644 index 0000000000..0de9c86d5d --- /dev/null +++ b/__tests__/fixtures/publish/public/package.json @@ -0,0 +1,7 @@ +{ + "name": "test", + "version": "0.0.0", + "publishConfig": { + "access": "public" + } +} diff --git a/src/cli/commands/publish.js b/src/cli/commands/publish.js index ee16431e4b..d53ed1ebe7 100644 --- a/src/cli/commands/publish.js +++ b/src/cli/commands/publish.js @@ -27,8 +27,15 @@ export function hasWrapper(commander: Object, args: Array): boolean { } async function publish(config: Config, pkg: any, flags: Object, dir: string): Promise { + let access = flags.access; + + // if no access level is provided, check package.json for `publishConfig.access` + // see: https://docs.npmjs.com/files/package.json#publishconfig + if (!access && pkg && pkg.publishConfig && pkg.publishConfig.access) { + access = pkg.publishConfig.access; + } + // validate access argument - const access = flags.access; if (access && access !== 'public' && access !== 'restricted') { throw new MessageError(config.reporter.lang('invalidAccess')); } @@ -69,7 +76,7 @@ async function publish(config: Config, pkg: any, flags: Object, dir: string): Pr // create body const root = { _id: pkg.name, - access: flags.access, + access, name: pkg.name, description: pkg.description, 'dist-tags': { From ba01ae4c1110d4243413b0998b8a4029f5146c33 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Mon, 29 Jan 2018 13:08:04 -0500 Subject: [PATCH 2/6] WIP: CI test failure debugging --- __tests__/commands/publish.js | 51 ++++++++++++++++++----------------- src/cli/commands/publish.js | 4 +++ 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/__tests__/commands/publish.js b/__tests__/commands/publish.js index 9575c21416..51727e806c 100644 --- a/__tests__/commands/publish.js +++ b/__tests__/commands/publish.js @@ -20,6 +20,9 @@ const runPublish = buildRun.bind( resolve({status: 200}); }), ); + // config.registries.yarn.config.username = 'test'; + // config.registries.yarn.config.email = 'test@yarnpkg.com'; + await publish(config, reporter, flags, args); return getStdout(); }, @@ -32,30 +35,30 @@ test.concurrent('publish should default access to undefined', () => { }); }); -test.concurrent('publish should accept `--access restricted` argument', () => { - return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'minimal', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('restricted'); - }); -}); +// test.concurrent('publish should accept `--access restricted` argument', () => { +// return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'minimal', config => { +// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; +// expect(requestCallParams.body.access).toEqual('restricted'); +// }); +// }); -test.concurrent('publish should accept `--access public` argument', () => { - return runPublish([], {newVersion: '0.0.1', access: 'public'}, 'minimal', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('public'); - }); -}); +// test.concurrent('publish should accept `--access public` argument', () => { +// return runPublish([], {newVersion: '0.0.1', access: 'public'}, 'minimal', config => { +// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; +// expect(requestCallParams.body.access).toEqual('public'); +// }); +// }); -test.concurrent('publish should use publishConfig.access in package manifest', () => { - return runPublish([], {newVersion: '0.0.1'}, 'public', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('public'); - }); -}); +// test.concurrent('publish should use publishConfig.access in package manifest', () => { +// return runPublish([], {newVersion: '0.0.1'}, 'public', config => { +// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; +// expect(requestCallParams.body.access).toEqual('public'); +// }); +// }); -test.concurrent('publish should allow `--access` to override publishConfig.access', () => { - return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'public', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('restricted'); - }); -}); +// test.concurrent('publish should allow `--access` to override publishConfig.access', () => { +// return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'public', config => { +// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; +// expect(requestCallParams.body.access).toEqual('restricted'); +// }); +// }); diff --git a/src/cli/commands/publish.js b/src/cli/commands/publish.js index d53ed1ebe7..f514ee3351 100644 --- a/src/cli/commands/publish.js +++ b/src/cli/commands/publish.js @@ -136,20 +136,24 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg } // + process.stdout.write('STEP 1\n'); reporter.step(1, 4, reporter.lang('bumpingVersion')); const commitVersion = await setVersion(config, reporter, flags, args, false); // + process.stdout.write.log('STEP 2\n'); reporter.step(2, 4, reporter.lang('loggingIn')); const revoke = await getToken(config, reporter, pkg.name); // + process.stdout.write.log('STEP 3\n'); reporter.step(3, 4, reporter.lang('publishing')); await publish(config, pkg, flags, dir); await commitVersion(); reporter.success(reporter.lang('published')); // + process.stdout.write.log('STEP 4\n'); reporter.step(4, 4, reporter.lang('revokingToken')); await revoke(); } From 2006898e8ad919be243169a66ebc49a5ee3053bd Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Mon, 29 Jan 2018 13:16:50 -0500 Subject: [PATCH 3/6] WIP: CI test failure debugging --- src/cli/commands/publish.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cli/commands/publish.js b/src/cli/commands/publish.js index f514ee3351..8810b25798 100644 --- a/src/cli/commands/publish.js +++ b/src/cli/commands/publish.js @@ -141,19 +141,19 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg const commitVersion = await setVersion(config, reporter, flags, args, false); // - process.stdout.write.log('STEP 2\n'); + process.stdout.write('STEP 2\n'); reporter.step(2, 4, reporter.lang('loggingIn')); const revoke = await getToken(config, reporter, pkg.name); // - process.stdout.write.log('STEP 3\n'); + process.stdout.write('STEP 3\n'); reporter.step(3, 4, reporter.lang('publishing')); await publish(config, pkg, flags, dir); await commitVersion(); reporter.success(reporter.lang('published')); // - process.stdout.write.log('STEP 4\n'); + process.stdout.write('STEP 4\n'); reporter.step(4, 4, reporter.lang('revokingToken')); await revoke(); } From 0efb23fca47260befb054c09ea2882ecacbcb65d Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Mon, 29 Jan 2018 13:44:24 -0500 Subject: [PATCH 4/6] WIP: CI test failure debugging --- __tests__/commands/publish.js | 6 ++++-- src/cli/commands/login.js | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/__tests__/commands/publish.js b/__tests__/commands/publish.js index 51727e806c..94aa31a18d 100644 --- a/__tests__/commands/publish.js +++ b/__tests__/commands/publish.js @@ -20,8 +20,10 @@ const runPublish = buildRun.bind( resolve({status: 200}); }), ); - // config.registries.yarn.config.username = 'test'; - // config.registries.yarn.config.email = 'test@yarnpkg.com'; + + // $FlowFixMe + config.registries.npm.getAuth = jest.fn(); + config.registries.npm.getAuth.mockReturnValue('test'); await publish(config, reporter, flags, args); return getStdout(); diff --git a/src/cli/commands/login.js b/src/cli/commands/login.js index 2b24434c75..9d826869c1 100644 --- a/src/cli/commands/login.js +++ b/src/cli/commands/login.js @@ -38,6 +38,7 @@ async function getCredentials( export async function getToken(config: Config, reporter: Reporter, name: string = ''): Promise<() => Promise> { const auth = config.registries.npm.getAuth(name); + console.log('GET TOKEN 1'); if (auth) { config.registries.npm.setToken(auth); return function revoke(): Promise { @@ -46,6 +47,7 @@ export async function getToken(config: Config, reporter: Reporter, name: string }; } + console.log('GET TOKEN 2'); const env = process.env.YARN_AUTH_TOKEN || process.env.NPM_AUTH_TOKEN; if (env) { config.registries.npm.setToken(`Bearer ${env}`); @@ -56,6 +58,7 @@ export async function getToken(config: Config, reporter: Reporter, name: string } // + console.log('GET TOKEN 3'); const creds = await getCredentials(config, reporter); if (!creds) { reporter.warn(reporter.lang('loginAsPublic')); @@ -66,6 +69,7 @@ export async function getToken(config: Config, reporter: Reporter, name: string } const {username, email} = creds; + console.log('PROMPT'); const password = await reporter.question(reporter.lang('npmPassword'), { password: true, required: true, From c0b644ed9d71c208493d75f8e808da49aa97d6a1 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Mon, 29 Jan 2018 14:22:27 -0500 Subject: [PATCH 5/6] fix CI errors by mocking npm password prompt --- __tests__/commands/publish.js | 77 +++++++++++++++++++---------------- src/cli/commands/login.js | 4 -- src/cli/commands/publish.js | 4 -- 3 files changed, 41 insertions(+), 44 deletions(-) diff --git a/__tests__/commands/publish.js b/__tests__/commands/publish.js index 94aa31a18d..f06d3e5bcd 100644 --- a/__tests__/commands/publish.js +++ b/__tests__/commands/publish.js @@ -8,23 +8,28 @@ const path = require('path'); const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'publish'); +const setupMocks = function(config) { + // Mock actual network request so that no package is actually published. + // $FlowFixMe + config.registries.npm.request = jest.fn(); + config.registries.npm.request.mockReturnValue( + new Promise(resolve => { + resolve({status: 200}); + }), + ); + + // Mock the npm login name. Otherwise yarn will prompt for it and break CI. + // $FlowFixMe + config.registries.npm.getAuth = jest.fn(); + config.registries.npm.getAuth.mockReturnValue('test'); +}; + const runPublish = buildRun.bind( null, ConsoleReporter, fixturesLoc, async (args, flags, config, reporter, lockfile, getStdout): Promise => { - // $FlowFixMe - config.registries.npm.request = jest.fn(); - config.registries.npm.request.mockReturnValue( - new Promise(resolve => { - resolve({status: 200}); - }), - ); - - // $FlowFixMe - config.registries.npm.getAuth = jest.fn(); - config.registries.npm.getAuth.mockReturnValue('test'); - + setupMocks(config); await publish(config, reporter, flags, args); return getStdout(); }, @@ -37,30 +42,30 @@ test.concurrent('publish should default access to undefined', () => { }); }); -// test.concurrent('publish should accept `--access restricted` argument', () => { -// return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'minimal', config => { -// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; -// expect(requestCallParams.body.access).toEqual('restricted'); -// }); -// }); +test.concurrent('publish should accept `--access restricted` argument', () => { + return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'minimal', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('restricted'); + }); +}); -// test.concurrent('publish should accept `--access public` argument', () => { -// return runPublish([], {newVersion: '0.0.1', access: 'public'}, 'minimal', config => { -// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; -// expect(requestCallParams.body.access).toEqual('public'); -// }); -// }); +test.concurrent('publish should accept `--access public` argument', () => { + return runPublish([], {newVersion: '0.0.1', access: 'public'}, 'minimal', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('public'); + }); +}); -// test.concurrent('publish should use publishConfig.access in package manifest', () => { -// return runPublish([], {newVersion: '0.0.1'}, 'public', config => { -// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; -// expect(requestCallParams.body.access).toEqual('public'); -// }); -// }); +test.concurrent('publish should use publishConfig.access in package manifest', () => { + return runPublish([], {newVersion: '0.0.1'}, 'public', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('public'); + }); +}); -// test.concurrent('publish should allow `--access` to override publishConfig.access', () => { -// return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'public', config => { -// const requestCallParams = config.registries.npm.request.mock.calls[0][1]; -// expect(requestCallParams.body.access).toEqual('restricted'); -// }); -// }); +test.concurrent('publish should allow `--access` to override publishConfig.access', () => { + return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'public', config => { + const requestCallParams = config.registries.npm.request.mock.calls[0][1]; + expect(requestCallParams.body.access).toEqual('restricted'); + }); +}); diff --git a/src/cli/commands/login.js b/src/cli/commands/login.js index 9d826869c1..2b24434c75 100644 --- a/src/cli/commands/login.js +++ b/src/cli/commands/login.js @@ -38,7 +38,6 @@ async function getCredentials( export async function getToken(config: Config, reporter: Reporter, name: string = ''): Promise<() => Promise> { const auth = config.registries.npm.getAuth(name); - console.log('GET TOKEN 1'); if (auth) { config.registries.npm.setToken(auth); return function revoke(): Promise { @@ -47,7 +46,6 @@ export async function getToken(config: Config, reporter: Reporter, name: string }; } - console.log('GET TOKEN 2'); const env = process.env.YARN_AUTH_TOKEN || process.env.NPM_AUTH_TOKEN; if (env) { config.registries.npm.setToken(`Bearer ${env}`); @@ -58,7 +56,6 @@ export async function getToken(config: Config, reporter: Reporter, name: string } // - console.log('GET TOKEN 3'); const creds = await getCredentials(config, reporter); if (!creds) { reporter.warn(reporter.lang('loginAsPublic')); @@ -69,7 +66,6 @@ export async function getToken(config: Config, reporter: Reporter, name: string } const {username, email} = creds; - console.log('PROMPT'); const password = await reporter.question(reporter.lang('npmPassword'), { password: true, required: true, diff --git a/src/cli/commands/publish.js b/src/cli/commands/publish.js index 8810b25798..d53ed1ebe7 100644 --- a/src/cli/commands/publish.js +++ b/src/cli/commands/publish.js @@ -136,24 +136,20 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg } // - process.stdout.write('STEP 1\n'); reporter.step(1, 4, reporter.lang('bumpingVersion')); const commitVersion = await setVersion(config, reporter, flags, args, false); // - process.stdout.write('STEP 2\n'); reporter.step(2, 4, reporter.lang('loggingIn')); const revoke = await getToken(config, reporter, pkg.name); // - process.stdout.write('STEP 3\n'); reporter.step(3, 4, reporter.lang('publishing')); await publish(config, pkg, flags, dir); await commitVersion(); reporter.success(reporter.lang('published')); // - process.stdout.write('STEP 4\n'); reporter.step(4, 4, reporter.lang('revokingToken')); await revoke(); } From f29e23585a12338c407f4b8e87356e9670621575 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Mon, 29 Jan 2018 15:28:56 -0500 Subject: [PATCH 6/6] use jest expect().toBeCalledWith() for publish command tests --- __tests__/commands/publish.js | 50 ++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/__tests__/commands/publish.js b/__tests__/commands/publish.js index f06d3e5bcd..aaee892112 100644 --- a/__tests__/commands/publish.js +++ b/__tests__/commands/publish.js @@ -37,35 +37,65 @@ const runPublish = buildRun.bind( test.concurrent('publish should default access to undefined', () => { return runPublish([], {newVersion: '0.0.1'}, 'minimal', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual(undefined); + expect(config.registries.npm.request).toBeCalledWith( + expect.any(String), + expect.objectContaining({ + body: expect.objectContaining({ + access: undefined, + }), + }), + ); }); }); test.concurrent('publish should accept `--access restricted` argument', () => { return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'minimal', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('restricted'); + expect(config.registries.npm.request).toBeCalledWith( + expect.any(String), + expect.objectContaining({ + body: expect.objectContaining({ + access: 'restricted', + }), + }), + ); }); }); test.concurrent('publish should accept `--access public` argument', () => { return runPublish([], {newVersion: '0.0.1', access: 'public'}, 'minimal', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('public'); + expect(config.registries.npm.request).toBeCalledWith( + expect.any(String), + expect.objectContaining({ + body: expect.objectContaining({ + access: 'public', + }), + }), + ); }); }); test.concurrent('publish should use publishConfig.access in package manifest', () => { return runPublish([], {newVersion: '0.0.1'}, 'public', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('public'); + expect(config.registries.npm.request).toBeCalledWith( + expect.any(String), + expect.objectContaining({ + body: expect.objectContaining({ + access: 'public', + }), + }), + ); }); }); test.concurrent('publish should allow `--access` to override publishConfig.access', () => { return runPublish([], {newVersion: '0.0.1', access: 'restricted'}, 'public', config => { - const requestCallParams = config.registries.npm.request.mock.calls[0][1]; - expect(requestCallParams.body.access).toEqual('restricted'); + expect(config.registries.npm.request).toBeCalledWith( + expect.any(String), + expect.objectContaining({ + body: expect.objectContaining({ + access: 'restricted', + }), + }), + ); }); });