fix(cleanup): move cli specific files to separate dir

lukekarrys · lukekarrys · commit 28019d50fdb1 · 2024-04-24T14:33:19.000-07:00
diff --git a/.eslintrc.local.js b/.eslintrc.local.js
@@ -25,8 +25,8 @@ module.exports = {
   overrides: es6Files({
     'index.js': ['lib/cli.js'],
     'bin/npm-cli.js': ['lib/cli.js'],
-    'lib/cli.js': ['lib/es6/validate-engines.js'],
-    'lib/es6/validate-engines.js': ['package.json'],
+    'lib/cli.js': ['lib/cli/validate-engines.js'],
+    'lib/cli/validate-engines.js': ['package.json'],
     // TODO: This file should also have its requires restricted as well since it
     // is an entry point but it currently pulls in config definitions which have
     // a large require graph, so that is not currently feasible. A future config
diff --git a/lib/cli.js b/lib/cli.js
@@ -1,4 +1,4 @@
-const validateEngines = require('./es6/validate-engines.js')
-const cliEntry = require('node:path').resolve(__dirname, 'cli-entry.js')
+const validateEngines = require('./cli/validate-engines.js')
+const cliEntry = require('node:path').resolve(__dirname, 'cli/entry.js')
 
 module.exports = (process) => validateEngines(process, () => require(cliEntry))
diff --git a/lib/cli/entry.js b/lib/cli/entry.js
@@ -11,9 +11,12 @@ module.exports = async (process, validateEngines) => {
     process.argv.splice(1, 1, 'npm', '-g')
   }
 
+  // Patch the global fs module here at the app level
+  require('graceful-fs').gracefulify(require('fs'))
+
   const satisfies = require('semver/functions/satisfies')
-  const exitHandler = require('./utils/exit-handler.js')
-  const Npm = require('./npm.js')
+  const exitHandler = require('./exit-handler.js')
+  const Npm = require('../npm.js')
   const npm = new Npm()
   exitHandler.setNpm(npm)
 
@@ -58,11 +61,29 @@ module.exports = async (process, validateEngines) => {
       return exitHandler()
     }
 
+    // this is async but we dont await it, since its ok if it doesnt
+    // finish before the command finishes running. it uses command and argv
+    // so it must be initiated here, after the command name is set
+    const updateNotifier = require('./update-notifier.js')
+    // eslint-disable-next-line promise/catch-or-return
+    updateNotifier(npm).then((msg) => (npm.updateNotification = msg))
+
+    // Options are prefixed by a hyphen-minus (-, \u2d).
+    // Other dash-type chars look similar but are invalid.
+    const nonDashArgs = npm.argv.filter(a => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(a))
+    if (nonDashArgs.length) {
+      log.error(
+        'arg',
+        'Argument starts with non-ascii dash, this is probably invalid:',
+        nonDashArgs.join(', ')
+      )
+    }
+
     await npm.exec(cmd)
     return exitHandler()
   } catch (err) {
     if (err.code === 'EUNKNOWNCOMMAND') {
-      const didYouMean = require('./utils/did-you-mean.js')
+      const didYouMean = require('../utils/did-you-mean.js')
       const suggestions = await didYouMean(npm.localPrefix, cmd)
       output.standard(`Unknown command: "${cmd}"${suggestions}\n`)
       output.standard('To see a list of supported npm commands, run:\n  npm help')
diff --git a/lib/cli/exit-handler.js b/lib/cli/exit-handler.js
@@ -1,5 +1,5 @@
 const { log, output, time } = require('proc-log')
-const errorMessage = require('./error-message.js')
+const errorMessage = require('../utils/error-message.js')
 const { redactLog: replaceInfo } = require('@npmcli/redact')
 
 let npm = null // set by the cli
diff --git a/lib/cli/update-notifier.js b/lib/cli/update-notifier.js
diff --git a/lib/cli/validate-engines.js b/lib/cli/validate-engines.js
diff --git a/lib/npm.js b/lib/npm.js
@@ -2,18 +2,13 @@ const { resolve, dirname, join } = require('node:path')
 const Config = require('@npmcli/config')
 const which = require('which')
 const fs = require('node:fs/promises')
-
-// Patch the global fs module here at the app level
-require('graceful-fs').gracefulify(require('fs'))
-
 const { definitions, flatten, shorthands } = require('@npmcli/config/lib/definitions')
 const usage = require('./utils/npm-usage.js')
 const LogFile = require('./utils/log-file.js')
 const Timers = require('./utils/timers.js')
 const Display = require('./utils/display.js')
 const { log, time } = require('proc-log')
 const { redactLog: replaceInfo } = require('@npmcli/redact')
-const updateNotifier = require('./utils/update-notifier.js')
 const pkg = require('../package.json')
 const { deref } = require('./utils/cmd-list.js')
 
@@ -42,7 +37,6 @@ class Npm {
   #title = 'npm'
   #argvClean = []
   #npmRoot = null
-  #warnedNonDashArg = false
 
   #chalk = null
   #logChalk = null
@@ -109,30 +103,7 @@ class Npm {
   // passed in directly.
   async exec (cmd, args = this.argv) {
     const command = this.setCmd(cmd)
-
-    const timeEnd = time.start(`command:${cmd}`)
-
-    // this is async but we dont await it, since its ok if it doesnt
-    // finish before the command finishes running. it uses command and argv
-    // so it must be initiated here, after the command name is set
-    // eslint-disable-next-line promise/catch-or-return
-    updateNotifier(this).then((msg) => (this.updateNotification = msg))
-
-    // Options are prefixed by a hyphen-minus (-, \u2d).
-    // Other dash-type chars look similar but are invalid.
-    if (!this.#warnedNonDashArg) {
-      const nonDashArgs = args.filter(a => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(a))
-      if (nonDashArgs.length) {
-        this.#warnedNonDashArg = true
-        log.error(
-          'arg',
-          'Argument starts with non-ascii dash, this is probably invalid:',
-          nonDashArgs.join(', ')
-        )
-      }
-    }
-
-    return command.cmdExec(args).finally(timeEnd)
+    return time.start(`command:${cmd}`, () => command.cmdExec(args))
   }
 
   async load () {
diff --git a/smoke-tests/test/npm-replace-global.js b/smoke-tests/test/npm-replace-global.js
@@ -190,7 +190,7 @@ t.test('fail when updating with lazy require', async t => {
   // exit-handler is the last thing called in the code
   // so an uncached lazy require within the exit handler will always throw
   await fs.writeFile(
-    join(paths.globalNodeModules, 'npm/lib/utils/exit-handler.js'),
+    join(paths.globalNodeModules, 'npm/lib/cli/exit-handler.js'),
     `module.exports = () => require('./LAZY_REQUIRE_CANARY');module.exports.setNpm = () => {}`,
     'utf-8'
   )
diff --git a/tap-snapshots/test/lib/cli/update-notifier.js.test.cjs b/tap-snapshots/test/lib/cli/update-notifier.js.test.cjs
@@ -5,95 +5,95 @@
  * Make sure to inspect the output below.  Do not ignore changes!
  */
 'use strict'
-exports[`test/lib/utils/update-notifier.js TAP notification situations 122.420.69 - color=always > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 122.420.69 - color=always > must match snapshot 1`] = `
 
 New [31mmajor[39m version of npm available! [31m122.420.69[39m -> [34m123.420.69[39m
 Changelog: [34mhttps://github.com/npm/cli/releases/tag/v123.420.69[39m
 To update run: [4mnpm install -g npm@123.420.69[24m
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 122.420.69 - color=false > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 122.420.69 - color=false > must match snapshot 1`] = `
 
 New major version of npm available! 122.420.69 -> 123.420.69
 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69
 To update run: npm install -g npm@123.420.69
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.419.69 - color=always > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.419.69 - color=always > must match snapshot 1`] = `
 
 New [33mminor[39m version of npm available! [33m123.419.69[39m -> [34m123.420.69[39m
 Changelog: [34mhttps://github.com/npm/cli/releases/tag/v123.420.69[39m
 To update run: [4mnpm install -g npm@123.420.69[24m
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.419.69 - color=false > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.419.69 - color=false > must match snapshot 1`] = `
 
 New minor version of npm available! 123.419.69 -> 123.420.69
 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69
 To update run: npm install -g npm@123.420.69
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.68 - color=always > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.68 - color=always > must match snapshot 1`] = `
 
 New [36mpatch[39m version of npm available! [36m123.420.68[39m -> [34m123.420.69[39m
 Changelog: [34mhttps://github.com/npm/cli/releases/tag/v123.420.69[39m
 To update run: [4mnpm install -g npm@123.420.69[24m
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.68 - color=false > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.68 - color=false > must match snapshot 1`] = `
 
 New patch version of npm available! 123.420.68 -> 123.420.69
 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69
 To update run: npm install -g npm@123.420.69
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.70 - color=always > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.70 - color=always > must match snapshot 1`] = `
 
 New [33mminor[39m version of npm available! [33m123.420.70[39m -> [34m123.421.70[39m
 Changelog: [34mhttps://github.com/npm/cli/releases/tag/v123.421.70[39m
 To update run: [4mnpm install -g npm@123.421.70[24m
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.70 - color=false > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.70 - color=false > must match snapshot 1`] = `
 
 New minor version of npm available! 123.420.70 -> 123.421.70
 Changelog: https://github.com/npm/cli/releases/tag/v123.421.70
 To update run: npm install -g npm@123.421.70
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.421.69 - color=always > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.421.69 - color=always > must match snapshot 1`] = `
 
 New [36mpatch[39m version of npm available! [36m123.421.69[39m -> [34m123.421.70[39m
 Changelog: [34mhttps://github.com/npm/cli/releases/tag/v123.421.70[39m
 To update run: [4mnpm install -g npm@123.421.70[24m
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 123.421.69 - color=false > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 123.421.69 - color=false > must match snapshot 1`] = `
 
 New patch version of npm available! 123.421.69 -> 123.421.70
 Changelog: https://github.com/npm/cli/releases/tag/v123.421.70
 To update run: npm install -g npm@123.421.70
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=always > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=always > must match snapshot 1`] = `
 
 New [36mprerelease[39m version of npm available! [36m124.0.0-beta.0[39m -> [34m124.0.0-beta.99999[39m
 Changelog: [34mhttps://github.com/npm/cli/releases/tag/v124.0.0-beta.99999[39m
 To update run: [4mnpm install -g npm@124.0.0-beta.99999[24m
 
 `
 
-exports[`test/lib/utils/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=false > must match snapshot 1`] = `
+exports[`test/lib/cli/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=false > must match snapshot 1`] = `
 
 New prerelease version of npm available! 124.0.0-beta.0 -> 124.0.0-beta.99999
 Changelog: https://github.com/npm/cli/releases/tag/v124.0.0-beta.99999
diff --git a/test/fixtures/mock-npm.js b/test/fixtures/mock-npm.js
@@ -48,17 +48,13 @@ const setGlobalNodeModules = (globalDir) => {
 }
 
 const buildMocks = (t, mocks) => {
-  const allMocks = {
-    '{LIB}/utils/update-notifier.js': async () => {},
-    ...mocks,
-  }
+  const allMocks = { ...mocks }
   // The definitions must be mocked since they are a singleton that reads from
   // process and environs to build defaults in order to break the requiure
   // cache. We also need to mock them with any mocks that were passed in for the
   // test in case those mocks are for things like ci-info which is used there.
   const definitions = '@npmcli/config/lib/definitions'
   allMocks[definitions] = tmock(t, definitions, allMocks)
-
   return allMocks
 }
 
diff --git a/test/lib/cli.js b/test/lib/cli.js
@@ -3,7 +3,7 @@ const tmock = require('../fixtures/tmock')
 
 t.test('returns cli-entry function', async t => {
   const cli = tmock(t, '{LIB}/cli.js', {
-    '{LIB}/cli-entry.js': () => 'ENTRY',
+    '{LIB}/cli/entry.js': () => 'ENTRY',
   })
 
   t.equal(cli(process), 'ENTRY')
diff --git a/test/lib/cli/entry.js b/test/lib/cli/entry.js
@@ -1,7 +1,7 @@
 const t = require('tap')
-const { load: loadMockNpm } = require('../fixtures/mock-npm.js')
-const tmock = require('../fixtures/tmock.js')
-const validateEngines = require('../../lib/es6/validate-engines.js')
+const { load: loadMockNpm } = require('../../fixtures/mock-npm.js')
+const tmock = require('../../fixtures/tmock.js')
+const validateEngines = require('../../../lib/cli/validate-engines.js')
 
 const cliMock = async (t, opts) => {
   let exitHandlerArgs = null
@@ -13,9 +13,9 @@ const cliMock = async (t, opts) => {
   exitHandlerMock.setNpm = _npm => npm = _npm
 
   const { Npm, ...mock } = await loadMockNpm(t, { ...opts, init: false })
-  const cli = tmock(t, '{LIB}/cli-entry.js', {
+  const cli = tmock(t, '{LIB}/cli/entry.js', {
     '{LIB}/npm.js': Npm,
-    '{LIB}/utils/exit-handler.js': exitHandlerMock,
+    '{LIB}/cli/exit-handler.js': exitHandlerMock,
   })
 
   return {
@@ -159,3 +159,16 @@ t.test('unsupported node version', async t => {
     /npm v.* does not support Node\.js 12\.6\.0\./
   )
 })
+
+t.test('non-ascii dash', async t => {
+  const { cli, logs } = await cliMock(t, {
+    globals: {
+      'process.argv': ['node', 'npm', 'scope', '\u2010not-a-dash'],
+    },
+  })
+  await cli(process)
+  t.equal(
+    logs.error[0],
+    'arg Argument starts with non-ascii dash, this is probably invalid: \u2010not-a-dash'
+  )
+})
diff --git a/test/lib/cli/update-notifier.js b/test/lib/cli/update-notifier.js
@@ -83,7 +83,7 @@ const runUpdateNotifier = async (t, {
     prefixDir,
     argv,
   })
-  const updateNotifier = tmock(t, '{LIB}/utils/update-notifier.js', mocks)
+  const updateNotifier = tmock(t, '{LIB}/cli/update-notifier.js', mocks)
 
   const result = await updateNotifier(mock.npm)
 
diff --git a/test/lib/cli/validate-engines.js b/test/lib/cli/validate-engines.js
@@ -3,7 +3,7 @@ const mockGlobals = require('@npmcli/mock-globals')
 const tmock = require('../../fixtures/tmock')
 
 const mockValidateEngines = (t) => {
-  const validateEngines = tmock(t, '{LIB}/es6/validate-engines.js', {
+  const validateEngines = tmock(t, '{LIB}/cli/validate-engines.js', {
     '{ROOT}/package.json': { version: '1.2.3', engines: { node: '>=0' } },
   })
   mockGlobals(t, { 'process.version': 'v4.5.6' })
diff --git a/test/lib/npm.js b/test/lib/npm.js
@@ -178,17 +178,16 @@ t.test('npm.load', async t => {
 
     outputs.length = 0
     logs.length = 0
-    await npm.exec('get', ['scope', '\u2010not-a-dash'])
+    await npm.exec('get', ['scope', 'usage'])
 
     t.strictSame([npm.command, npm.flatOptions.npmCommand], ['ll', 'll'],
       'does not change npm.command when another command is called')
 
     t.match(logs, [
-      'error arg Argument starts with non-ascii dash, this is probably invalid: \u2010not-a-dash',
       /timing command:config Completed in [0-9.]+ms/,
       /timing command:get Completed in [0-9.]+ms/,
     ])
-    t.same(outputs, ['scope=@foo\n\u2010not-a-dash=undefined'])
+    t.same(outputs, ['scope=@foo\nusage=false'])
   })
 
   await t.test('--no-workspaces with --workspace', async t => {
diff --git a/test/lib/utils/exit-handler.js b/test/lib/utils/exit-handler.js
@@ -72,7 +72,7 @@ const mockExitHandler = async (t, { config, mocks, files, ...opts } = {}) => {
     },
   })
 
-  const exitHandler = tmock(t, '{LIB}/utils/exit-handler.js', {
+  const exitHandler = tmock(t, '{LIB}/cli/exit-handler.js', {
     '{LIB}/utils/error-message.js': (err) => ({
       summary: [['ERR SUMMARY', err.message]],
       detail: [['ERR DETAIL', err.message]],
diff --git a/test/lib/utils/installed-deep.js b/test/lib/utils/installed-deep.js
@@ -1,6 +1,6 @@
 const t = require('tap')
 const installedDeep = require('../../../lib/utils/installed-deep.js')
-const mockNpm = require('../../../fixtures/mock-npm')
+const mockNpm = require('../../fixtures/mock-npm')
 
 const fixture = {
   'package.json': JSON.stringify({
diff --git a/test/lib/utils/installed-shallow.js b/test/lib/utils/installed-shallow.js
@@ -1,6 +1,6 @@
 const t = require('tap')
 const installed = require('../../../lib/utils/installed-shallow.js')
-const mockNpm = require('../../../fixtures/mock-npm')
+const mockNpm = require('../../fixtures/mock-npm')
 
 const mockShallow = async (t, config) => {
   const res = await mockNpm(t, {

Original file line number	Diff line number	Diff line change
`@@ -190,7 +190,7 @@ t.test('fail when updating with lazy require', async t => {`
`190`	`190`	`// exit-handler is the last thing called in the code`
`191`	`191`	`// so an uncached lazy require within the exit handler will always throw`
`192`	`192`	`await fs.writeFile(`
`193`		`- join(paths.globalNodeModules, 'npm/lib/utils/exit-handler.js'),`
	`193`	`+ join(paths.globalNodeModules, 'npm/lib/cli/exit-handler.js'),`
`194`	`194`	`module.exports = () => require('./LAZY_REQUIRE_CANARY');module.exports.setNpm = () => {}`,
`195`	`195`	`'utf-8'`
`196`	`196`	`)`