From eba203f87066e535c5956f07e814cb0e828ce54d Mon Sep 17 00:00:00 2001 From: Tommy Date: Thu, 6 Apr 2023 09:38:10 -0500 Subject: [PATCH] Improve startup: ensure `package.json` is only parsed once (#688) --- source/cli-implementation.js | 14 +++++------ source/config.js | 5 ++-- source/git-util.js | 16 ++++--------- source/index.js | 7 ++---- source/npm/util.js | 14 ++++------- source/ui.js | 20 ++++++++-------- source/util.js | 19 +++++++-------- test/config.js | 6 ++--- test/index.js | 11 +++++---- test/integration.js | 15 ++++++------ test/{new-files.js => packed-files.js} | 32 +++++++++++--------------- 11 files changed, 70 insertions(+), 89 deletions(-) rename test/{new-files.js => packed-files.js} (50%) diff --git a/source/cli-implementation.js b/source/cli-implementation.js index 3b394f92..5ccdf694 100755 --- a/source/cli-implementation.js +++ b/source/cli-implementation.js @@ -7,10 +7,10 @@ import updateNotifier from 'update-notifier'; import hasYarn from 'has-yarn'; import {gracefulExit} from 'exit-hook'; import config from './config.js'; +import * as util from './util.js'; import * as git from './git-util.js'; -import {isPackageNameAvailable} from './npm/util.js'; +import * as npm from './npm/util.js'; import Version from './version.js'; -import * as util from './util.js'; import ui from './ui.js'; import np from './index.js'; @@ -99,7 +99,7 @@ const cli = meow(` updateNotifier({pkg: cli.pkg}).notify(); try { - const {pkg, pkgPath} = await util.readPkg(); + const {pkg, rootDir} = await util.readPkg(cli.flags.contents); const defaultFlags = { cleanup: true, @@ -110,7 +110,7 @@ try { '2fa': true, }; - const localConfig = await config(); + const localConfig = await config(rootDir); const flags = { ...defaultFlags, @@ -125,7 +125,7 @@ try { const runPublish = !flags.releaseDraftOnly && flags.publish && !pkg.private; - const availability = flags.publish ? await isPackageNameAvailable(pkg) : { + const availability = flags.publish ? await npm.isPackageNameAvailable(pkg) : { isAvailable: false, isUnknown: false, }; @@ -140,14 +140,14 @@ try { version, runPublish, branch, - }, {pkg, pkgPath}); + }, {pkg, rootDir}); if (!options.confirm) { gracefulExit(); } console.log(); // Prints a newline for readability - const newPkg = await np(options.version, options); + const newPkg = await np(options.version, options, {pkg, rootDir}); if (options.preview || options.releaseDraftOnly) { gracefulExit(); diff --git a/source/config.js b/source/config.js index cd78f7b8..954b66d8 100644 --- a/source/config.js +++ b/source/config.js @@ -1,6 +1,5 @@ import os from 'node:os'; import isInstalledGlobally from 'is-installed-globally'; -import {packageDirectory} from 'pkg-dir'; import {cosmiconfig} from 'cosmiconfig'; // TODO: remove when cosmiconfig/cosmiconfig#283 lands @@ -9,8 +8,8 @@ const loadESM = async filepath => { return module.default ?? module; }; -const getConfig = async () => { - const searchDir = isInstalledGlobally ? os.homedir() : await packageDirectory(); +const getConfig = async rootDir => { + const searchDir = isInstalledGlobally ? os.homedir() : rootDir; const searchPlaces = ['.np-config.json', '.np-config.js', '.np-config.cjs', '.np-config.mjs']; if (!isInstalledGlobally) { searchPlaces.push('package.json'); diff --git a/source/git-util.js b/source/git-util.js index 207a145f..7b114cb8 100644 --- a/source/git-util.js +++ b/source/git-util.js @@ -2,7 +2,6 @@ import path from 'node:path'; import {execa} from 'execa'; import escapeStringRegexp from 'escape-string-regexp'; import ignoreWalker from 'ignore-walk'; -import {packageDirectory} from 'pkg-dir'; import Version from './version.js'; export const latestTag = async () => { @@ -15,7 +14,7 @@ export const root = async () => { return stdout; }; -export const newFilesSinceLastRelease = async () => { +export const newFilesSinceLastRelease = async rootDir => { try { const {stdout} = await execa('git', ['diff', '--name-only', '--diff-filter=A', await latestTag(), 'HEAD']); if (stdout.trim().length === 0) { @@ -27,7 +26,7 @@ export const newFilesSinceLastRelease = async () => { } catch { // Get all files under version control return ignoreWalker({ - path: await packageDirectory(), + path: rootDir, ignoreFiles: ['.gitignore'], }); } @@ -205,12 +204,7 @@ export const tagExistsOnRemote = async tagName => { async function hasLocalBranch(branch) { try { - await execa('git', [ - 'show-ref', - '--verify', - '--quiet', - `refs/heads/${branch}`, - ]); + await execa('git', ['show-ref', '--verify', '--quiet', `refs/heads/${branch}`]); return true; } catch { return false; @@ -225,9 +219,7 @@ export const defaultBranch = async () => { } } - throw new Error( - 'Could not infer the default Git branch. Please specify one with the --branch flag or with a np config.', - ); + throw new Error('Could not infer the default Git branch. Please specify one with the --branch flag or with a np config.'); }; export const verifyTagDoesNotExistOnRemote = async tagName => { diff --git a/source/index.js b/source/index.js index a3663b01..c50319b5 100644 --- a/source/index.js +++ b/source/index.js @@ -6,7 +6,6 @@ import Listr from 'listr'; import {merge, throwError, catchError, filter, finalize} from 'rxjs'; import {readPackageUp} from 'read-pkg-up'; import hasYarn from 'has-yarn'; -import {packageDirectorySync} from 'pkg-dir'; import hostedGitInfo from 'hosted-git-info'; import onetime from 'onetime'; import {asyncExitHook} from 'exit-hook'; @@ -15,10 +14,10 @@ import prerequisiteTasks from './prerequisite-tasks.js'; import gitTasks from './git-tasks.js'; import publish from './npm/publish.js'; import enable2fa from './npm/enable-2fa.js'; -import * as npm from './npm/util.js'; import releaseTaskHelper from './release-task-helper.js'; import * as util from './util.js'; import * as git from './git-util.js'; +import * as npm from './npm/util.js'; const exec = (cmd, args) => { // Use `Observable` support if merged https://github.com/sindresorhus/execa/pull/26 @@ -28,7 +27,7 @@ const exec = (cmd, args) => { }; // eslint-disable-next-line complexity -const np = async (input = 'patch', options) => { +const np = async (input = 'patch', options, {pkg, rootDir}) => { if (!hasYarn() && options.yarn) { throw new Error('Could not use Yarn without yarn.lock file'); } @@ -38,12 +37,10 @@ const np = async (input = 'patch', options) => { options.cleanup = false; } - const {pkg} = await util.readPkg(options.contents); const runTests = options.tests && !options.yolo; const runCleanup = options.cleanup && !options.yolo; const pkgManager = options.yarn === true ? 'yarn' : 'npm'; const pkgManagerName = options.yarn === true ? 'Yarn' : 'npm'; - const rootDir = packageDirectorySync(); const hasLockFile = fs.existsSync(path.resolve(rootDir, options.yarn ? 'yarn.lock' : 'package-lock.json')) || fs.existsSync(path.resolve(rootDir, 'npm-shrinkwrap.json')); const isOnGitHub = options.repoUrl && hostedGitInfo.fromUrl(options.repoUrl)?.type === 'github'; const testScript = options.testScript || 'test'; diff --git a/source/npm/util.js b/source/npm/util.js index 7cf46998..fec23fd9 100644 --- a/source/npm/util.js +++ b/source/npm/util.js @@ -5,7 +5,6 @@ import pTimeout from 'p-timeout'; import ow from 'ow'; import npmName from 'npm-name'; import chalk from 'chalk'; -import {packageDirectory} from 'pkg-dir'; import semver from 'semver'; import Version from '../version.js'; @@ -129,21 +128,18 @@ export const verifyRecentNpmVersion = async () => { Version.verifyRequirementSatisfied('npm', npmVersion); }; -const npmignoreExistsInPackageRootDir = async () => { - const rootDir = await packageDirectory(); - return pathExists(path.resolve(rootDir, '.npmignore')); -}; +export const checkIgnoreStrategy = ({files}, rootDir) => { + const npmignoreExistsInPackageRootDir = pathExists(path.resolve(rootDir, '.npmignore')); -export const checkIgnoreStrategy = async ({files}) => { - if (!files && !(await npmignoreExistsInPackageRootDir())) { + if (!files && !npmignoreExistsInPackageRootDir) { console.log(` \n${chalk.bold.yellow('Warning:')} No ${chalk.bold.cyan('files')} field specified in ${chalk.bold.magenta('package.json')} nor is a ${chalk.bold.magenta('.npmignore')} file present. Having one of those will prevent you from accidentally publishing development-specific files along with your package's source code to npm. `); } }; -export const getFilesToBePacked = async () => { - const {stdout} = await execa('npm', ['pack', '--dry-run', '--json'], {cwd: await packageDirectory()}); +export const getFilesToBePacked = async rootDir => { + const {stdout} = await execa('npm', ['pack', '--dry-run', '--json'], {cwd: rootDir}); const {files} = JSON.parse(stdout).at(0); return files.map(file => file.path); diff --git a/source/ui.js b/source/ui.js index 389cf53e..1e60e51d 100644 --- a/source/ui.js +++ b/source/ui.js @@ -6,7 +6,7 @@ import isScoped from 'is-scoped'; import isInteractive from 'is-interactive'; import * as util from './util.js'; import * as git from './git-util.js'; -import * as npmUtil from './npm/util.js'; +import * as npm from './npm/util.js'; import Version from './version.js'; import prettyVersionDiff from './pretty-version-diff.js'; @@ -78,9 +78,9 @@ const printCommitLog = async (repoUrl, registryUrl, fromLatestTag, releaseBranch }; }; -const checkNewFilesAndDependencies = async (pkg, pkgPath) => { - const newFiles = await util.getNewFiles(pkg); - const newDependencies = await util.getNewDependencies(pkg, pkgPath); +const checkNewFilesAndDependencies = async (pkg, rootDir) => { + const newFiles = await util.getNewFiles(rootDir); + const newDependencies = await util.getNewDependencies(pkg, rootDir); const noNewUnpublishedFiles = !newFiles.unpublished || newFiles.unpublished.length === 0; const noNewFirstTimeFiles = !newFiles.firstTime || newFiles.firstTime.length === 0; @@ -121,18 +121,18 @@ const checkNewFilesAndDependencies = async (pkg, pkgPath) => { }; // eslint-disable-next-line complexity -const ui = async (options, {pkg, pkgPath}) => { +const ui = async (options, {pkg, rootDir}) => { const oldVersion = pkg.version; const extraBaseUrls = ['gitlab.com']; const repoUrl = pkg.repository && githubUrlFromGit(pkg.repository.url, {extraBaseUrls}); const pkgManager = options.yarn ? 'yarn' : 'npm'; - const registryUrl = await npmUtil.getRegistryUrl(pkgManager, pkg); + const registryUrl = await npm.getRegistryUrl(pkgManager, pkg); const releaseBranch = options.branch; if (options.runPublish) { - await npmUtil.checkIgnoreStrategy(pkg); + npm.checkIgnoreStrategy(pkg, rootDir); - const answerIgnoredFiles = await checkNewFilesAndDependencies(pkg, pkgPath); + const answerIgnoredFiles = await checkNewFilesAndDependencies(pkg, rootDir); if (!answerIgnoredFiles) { return { ...options, @@ -253,7 +253,7 @@ const ui = async (options, {pkg, pkgPath}) => { message: 'How should this pre-release version be tagged in npm?', when: answers => options.runPublish && (Version.isPrereleaseOrIncrement(answers.customVersion) || Version.isPrereleaseOrIncrement(answers.version)) && !options.tag, async choices() { - const existingPrereleaseTags = await npmUtil.prereleaseTags(pkg.name); + const existingPrereleaseTags = await npm.prereleaseTags(pkg.name); return [ ...existingPrereleaseTags, @@ -283,7 +283,7 @@ const ui = async (options, {pkg, pkgPath}) => { }, publishScoped: { type: 'confirm', - when: isScoped(pkg.name) && options.availability.isAvailable && !options.availability.isUnknown && options.runPublish && (pkg.publishConfig && pkg.publishConfig.access !== 'restricted') && !npmUtil.isExternalRegistry(pkg), + when: isScoped(pkg.name) && options.availability.isAvailable && !options.availability.isUnknown && options.runPublish && (pkg.publishConfig && pkg.publishConfig.access !== 'restricted') && !npm.isExternalRegistry(pkg), message: `This scoped repo ${chalk.bold.magenta(pkg.name)} hasn't been published. Do you want to publish it publicly?`, default: false, }, diff --git a/source/util.js b/source/util.js index 88960b55..dda0f951 100644 --- a/source/util.js +++ b/source/util.js @@ -1,3 +1,4 @@ +import path from 'node:path'; import {readPackageUp} from 'read-pkg-up'; import issueRegex from 'issue-regex'; import terminalLink from 'terminal-link'; @@ -6,8 +7,8 @@ import pMemoize from 'p-memoize'; import ow from 'ow'; import chalk from 'chalk'; import {packageDirectory} from 'pkg-dir'; -import * as gitUtil from './git-util.js'; -import * as npmUtil from './npm/util.js'; +import * as git from './git-util.js'; +import * as npm from './npm/util.js'; export const readPkg = async packagePath => { packagePath = packagePath ? await packageDirectory(packagePath) : await packageDirectory(); @@ -15,11 +16,11 @@ export const readPkg = async packagePath => { throw new Error('No `package.json` found. Make sure the current directory is a valid package.'); } - const {packageJson, path} = await readPackageUp({ + const {packageJson, path: pkgPath} = await readPackageUp({ cwd: packagePath, }); - return {pkg: packageJson, pkgPath: path}; + return {pkg: packageJson, rootDir: path.dirname(pkgPath)}; }; export const linkifyIssues = (url, message) => { @@ -72,9 +73,9 @@ export const getTagVersionPrefix = pMemoize(async options => { export const joinList = list => chalk.reset(list.map(item => `- ${item}`).join('\n')); -export const getNewFiles = async () => { - const listNewFiles = await gitUtil.newFilesSinceLastRelease(); - const listPkgFiles = await npmUtil.getFilesToBePacked(); +export const getNewFiles = async rootDir => { + const listNewFiles = await git.newFilesSinceLastRelease(rootDir); + const listPkgFiles = await npm.getFilesToBePacked(rootDir); return { unpublished: listNewFiles.filter(file => !listPkgFiles.includes(file) && !file.startsWith('.git')), @@ -82,8 +83,8 @@ export const getNewFiles = async () => { }; }; -export const getNewDependencies = async (newPkg, pkgPath) => { - let oldPkg = await gitUtil.readFileFromLastRelease(pkgPath); +export const getNewDependencies = async (newPkg, rootDir) => { + let oldPkg = await git.readFileFromLastRelease(path.resolve(rootDir, 'package.json')); oldPkg = JSON.parse(oldPkg); const newDependencies = []; diff --git a/test/config.js b/test/config.js index 0800c14d..de56e1d9 100644 --- a/test/config.js +++ b/test/config.js @@ -14,10 +14,9 @@ const getConfigsWhenGlobalBinaryIsUsed = async homedirStub => { const promises = pathsPkgDir.map(async pathPkgDir => { const getConfig = await esmock(testedModulePath, { 'is-installed-globally': true, - 'pkg-dir': {packageDirectory: async () => pathPkgDir}, 'node:os': {homedir: homedirStub}, }); - return getConfig(); + return getConfig(pathPkgDir); }); return Promise.all(promises); @@ -29,10 +28,9 @@ const getConfigsWhenLocalBinaryIsUsed = async pathPkgDir => { const promises = homedirs.map(async homedir => { const getConfig = await esmock(testedModulePath, { 'is-installed-globally': false, - 'pkg-dir': {packageDirectory: async () => pathPkgDir}, 'node:os': {homedir: () => homedir}, }); - return getConfig(); + return getConfig(pathPkgDir); }); return Promise.all(promises); diff --git a/test/index.js b/test/index.js index dbf61745..03a0660c 100644 --- a/test/index.js +++ b/test/index.js @@ -1,6 +1,7 @@ import test from 'ava'; import sinon from 'sinon'; import esmock from 'esmock'; +import * as util from '../source/util.js'; import np from '../source/index.js'; const defaultOptions = { @@ -15,9 +16,11 @@ const defaultOptions = { renderer: 'silent', }; +const npPkg = await util.readPkg(); + const npFails = test.macro(async (t, inputs, message) => { await t.throwsAsync( - Promise.all(inputs.map(input => np(input, defaultOptions))), + Promise.all(inputs.map(input => np(input, defaultOptions, npPkg))), {message}, ); }); @@ -51,7 +54,7 @@ test('skip enabling 2FA if the package exists', async t => { }, '../source/npm/enable-2fa.js': enable2faStub, '../source/npm/publish.js': sinon.stub().returns({pipe: sinon.stub()}), - }, {}); + }); await t.notThrowsAsync(npMock('1.0.0', { ...defaultOptions, @@ -59,7 +62,7 @@ test('skip enabling 2FA if the package exists', async t => { isAvailable: false, isUnknown: false, }, - })); + }, npPkg)); t.true(enable2faStub.notCalled); }); @@ -87,7 +90,7 @@ test('skip enabling 2FA if the `2fa` option is false', async t => { isUnknown: false, }, '2fa': false, - })); + }, npPkg)); t.true(enable2faStub.notCalled); }); diff --git a/test/integration.js b/test/integration.js index 25abc62b..fab34722 100644 --- a/test/integration.js +++ b/test/integration.js @@ -41,13 +41,12 @@ test('main', async t => { }); }); -const createFixture = test.macro(async (t, pkgFiles, commands, {unpublished, firstTime}) => { +const createNewFilesFixture = test.macro(async (t, pkgFiles, commands, {unpublished, firstTime}) => { await createIntegrationTest(t, async ($$, temporaryDir) => { /** @type {import('../source/util.js')} */ const util = await esmock('../source/util.js', {}, { 'node:process': {cwd: () => temporaryDir}, execa: {execa: async (...args) => execa(...args, {cwd: temporaryDir})}, - 'pkg-dir': {packageDirectory: async () => temporaryDir}, }); await commands(t, $$, temporaryDir); @@ -59,13 +58,13 @@ const createFixture = test.macro(async (t, pkgFiles, commands, {unpublished, fir }); t.deepEqual( - await util.getNewFiles(), + await util.getNewFiles(temporaryDir), {unpublished, firstTime}, ); }); }); -test('files to package with tags added', createFixture, ['*.js'], async (t, $$) => { +test('files to package with tags added', createNewFilesFixture, ['*.js'], async (t, $$) => { await $$`git tag v0.0.0`; await t.context.createFile('new'); await t.context.createFile('index.js'); @@ -73,7 +72,7 @@ test('files to package with tags added', createFixture, ['*.js'], async (t, $$) await $$`git commit -m "added"`; }, {unpublished: ['new'], firstTime: ['index.js']}); -test('file `new` to package without tags added', createFixture, ['index.js'], async t => { +test('file `new` to package without tags added', createNewFilesFixture, ['index.js'], async t => { await t.context.createFile('new'); await t.context.createFile('index.js'); }, {unpublished: ['new'], firstTime: ['index.js', 'package.json']}); @@ -83,7 +82,7 @@ test('file `new` to package without tags added', createFixture, ['index.js'], as const filePath1 = path.join(longPath, 'file1'); const filePath2 = path.join(longPath, 'file2'); - test('files with long pathnames added', createFixture, ['*.js'], async (t, $$) => { + test('files with long pathnames added', createNewFilesFixture, ['*.js'], async (t, $$) => { await $$`git tag v0.0.0`; await t.context.createFile(filePath1); await t.context.createFile(filePath2); @@ -92,11 +91,11 @@ test('file `new` to package without tags added', createFixture, ['index.js'], as }, {unpublished: [filePath1, filePath2], firstTime: []}); })(); -test('no new files added', createFixture, [], async (_t, $$) => { +test('no new files added', createNewFilesFixture, [], async (_t, $$) => { await $$`git tag v0.0.0`; }, {unpublished: [], firstTime: []}); -test('ignores .git and .github files', createFixture, ['*.js'], async (t, $$) => { +test('ignores .git and .github files', createNewFilesFixture, ['*.js'], async (t, $$) => { await $$`git tag v0.0.0`; await t.context.createFile('.github/workflows/main.yml'); await t.context.createFile('.github/pull_request_template'); diff --git a/test/new-files.js b/test/packed-files.js similarity index 50% rename from test/new-files.js rename to test/packed-files.js index 9b1046d2..b2a44aad 100644 --- a/test/new-files.js +++ b/test/packed-files.js @@ -1,56 +1,52 @@ import path from 'node:path'; import test from 'ava'; -import esmock from 'esmock'; import {renameFile} from 'move-file'; +import {getFilesToBePacked} from '../source/npm/util.js'; import {runIfExists} from './_utils.js'; const getFixture = name => path.resolve('test', 'fixtures', 'files', name); -const mockPkgDir = test.macro(async (t, fixture, expectedFiles, {before, after} = {}) => { +const verifyPackedFiles = test.macro(async (t, fixture, expectedFiles, {before, after} = {}) => { const fixtureDir = getFixture(fixture); await runIfExists(before, fixtureDir); t.teardown(async () => runIfExists(after, fixtureDir)); - const npmUtil = await esmock('../source/npm/util.js', { - 'pkg-dir': {packageDirectory: async () => fixtureDir}, - }); - - const files = await npmUtil.getFilesToBePacked(); + const files = await getFilesToBePacked(fixtureDir); t.deepEqual(files.sort(), [...expectedFiles, 'package.json'].sort(), 'Files different from expectations!'); }); -test('package.json files field - one file', mockPkgDir, 'one-file', [ +test('package.json files field - one file', verifyPackedFiles, 'one-file', [ 'index.js', ]); -test('package.json files field - source dir', mockPkgDir, 'source-dir', [ +test('package.json files field - source dir', verifyPackedFiles, 'source-dir', [ 'source/foo.js', 'source/bar.js', ]); -test('package.json files field - source and dist dirs', mockPkgDir, 'source-and-dist-dir', [ +test('package.json files field - source and dist dirs', verifyPackedFiles, 'source-and-dist-dir', [ 'source/foo.js', 'source/bar.js', ]); -test('package.json files field - leading slash', mockPkgDir, 'files-slash', [ +test('package.json files field - leading slash', verifyPackedFiles, 'files-slash', [ 'index.js', ]); -test('package.json files field - has readme and license', mockPkgDir, 'has-readme-and-license', [ +test('package.json files field - has readme and license', verifyPackedFiles, 'has-readme-and-license', [ 'readme.md', 'license.md', 'index.js', ]); -test('npmignore', mockPkgDir, 'npmignore', [ +test('npmignore', verifyPackedFiles, 'npmignore', [ 'readme.md', 'index.js', 'index.d.ts', ]); -test('package.json files field and npmignore', mockPkgDir, 'files-and-npmignore', [ +test('package.json files field and npmignore', verifyPackedFiles, 'files-and-npmignore', [ 'readme.md', 'source/foo.js', 'source/bar.js', @@ -66,22 +62,22 @@ const renameDotGitignore = { }, }; -test('package.json files field and gitignore', mockPkgDir, 'gitignore', [ +test('package.json files field and gitignore', verifyPackedFiles, 'gitignore', [ 'readme.md', 'dist/index.js', ], renameDotGitignore); -test('npmignore and gitignore', mockPkgDir, 'npmignore-and-gitignore', [ +test('npmignore and gitignore', verifyPackedFiles, 'npmignore-and-gitignore', [ 'readme.md', 'dist/index.js', ], renameDotGitignore); -test('package.json main field not in files field', mockPkgDir, 'main', [ +test('package.json main field not in files field', verifyPackedFiles, 'main', [ 'foo.js', 'bar.js', ]); -test('doesn\'t show files in .github', mockPkgDir, 'dot-github', [ +test('doesn\'t show files in .github', verifyPackedFiles, 'dot-github', [ 'index.js', ]);