From 926f0adbd71949c905932a241a245b78c85ef643 Mon Sep 17 00:00:00 2001
From: Gar <gar+gh@danger.computer>
Date: Thu, 8 Sep 2022 09:51:57 -0700
Subject: [PATCH] feat: remove `npm set-script` (#5456)

BREAKING CHANGE: this removes `npm set-script`

Folks should use `npm pkg set` to set the `scripts` field in their
`package.json`

Closes https://github.com/npm/statusboard/issues/449
---
 lib/commands/set-script.js                    |  96 ---------
 lib/utils/cmd-list.js                         |   1 -
 .../tap-snapshots/test/index.js.test.cjs      |  38 ++--
 smoke-tests/test/index.js                     |   4 +-
 .../test/lib/commands/completion.js.test.cjs  |   1 -
 .../test/lib/load-all-commands.js.test.cjs    |  13 --
 tap-snapshots/test/lib/npm.js.test.cjs        |  35 ++--
 .../test/lib/utils/cmd-list.js.test.cjs       |   8 -
 test/lib/commands/set-script.js               | 188 ------------------
 9 files changed, 33 insertions(+), 351 deletions(-)
 delete mode 100644 lib/commands/set-script.js
 delete mode 100644 test/lib/commands/set-script.js

diff --git a/lib/commands/set-script.js b/lib/commands/set-script.js
deleted file mode 100644
index a085f72a31428..0000000000000
--- a/lib/commands/set-script.js
+++ /dev/null
@@ -1,96 +0,0 @@
-const { resolve } = require('path')
-const rpj = require('read-package-json-fast')
-const PackageJson = require('@npmcli/package-json')
-const log = require('../utils/log-shim')
-
-const BaseCommand = require('../base-command.js')
-class SetScript extends BaseCommand {
-  static description = 'Set tasks in the scripts section of package.json, deprecated'
-  static params = ['workspace', 'workspaces', 'include-workspace-root']
-  static name = 'set-script'
-  static usage = ['[<script>] [<command>]']
-  static ignoreImplicitWorkspace = false
-
-  async completion (opts) {
-    const argv = opts.conf.argv.remain
-    if (argv.length === 2) {
-      // find the script name
-      const json = resolve(this.npm.localPrefix, 'package.json')
-      const { scripts = {} } = await rpj(json).catch(er => ({}))
-      return Object.keys(scripts)
-    }
-  }
-
-  validate (args) {
-    if (process.env.npm_lifecycle_event === 'postinstall') {
-      throw new Error('Scripts can’t set from the postinstall script')
-    }
-
-    // Parse arguments
-    if (args.length !== 2) {
-      throw new Error(`Expected 2 arguments: got ${args.length}`)
-    }
-  }
-
-  async exec (args) {
-    this.validate(args)
-    log.warn('set-script',
-      'set-script is deprecated, use `npm pkg set scripts.scriptname="cmd" instead.')
-    const warn = await this.doSetScript(this.npm.localPrefix, args[0], args[1])
-    if (warn) {
-      log.warn('set-script', `Script "${args[0]}" was overwritten`)
-    }
-  }
-
-  async execWorkspaces (args, filters) {
-    this.validate(args)
-    await this.setWorkspaces(filters)
-
-    for (const [name, path] of this.workspaces) {
-      try {
-        const warn = await this.doSetScript(path, args[0], args[1])
-        if (warn) {
-          log.warn('set-script', `Script "${args[0]}" was overwritten`)
-          log.warn(`  in workspace: ${name}`)
-          log.warn(`  at location: ${path}`)
-        }
-      } catch (err) {
-        log.error('set-script', err.message)
-        log.error(`  in workspace: ${name}`)
-        log.error(`  at location: ${path}`)
-        process.exitCode = 1
-      }
-    }
-  }
-
-  // returns a Boolean that will be true if
-  // the requested script was overwritten
-  // and false if it was set as a new script
-  async doSetScript (path, name, value) {
-    let warn = false
-
-    const pkgJson = await PackageJson.load(path)
-    const { scripts } = pkgJson.content
-
-    const overwriting =
-      scripts
-        && scripts[name]
-        && scripts[name] !== value
-
-    if (overwriting) {
-      warn = true
-    }
-
-    pkgJson.update({
-      scripts: {
-        ...scripts,
-        [name]: value,
-      },
-    })
-
-    await pkgJson.save()
-
-    return warn
-  }
-}
-module.exports = SetScript
diff --git a/lib/utils/cmd-list.js b/lib/utils/cmd-list.js
index 4dca7df2d304b..d74f9878fbd15 100644
--- a/lib/utils/cmd-list.js
+++ b/lib/utils/cmd-list.js
@@ -121,7 +121,6 @@ const cmdList = [
   'run-script',
   'search',
   'set',
-  'set-script',
   'shrinkwrap',
   'star',
   'stars',
diff --git a/smoke-tests/tap-snapshots/test/index.js.test.cjs b/smoke-tests/tap-snapshots/test/index.js.test.cjs
index f7e49ed91e1d2..017ca71d37777 100644
--- a/smoke-tests/tap-snapshots/test/index.js.test.cjs
+++ b/smoke-tests/tap-snapshots/test/index.js.test.cjs
@@ -27,9 +27,9 @@ All commands:
     hook, init, install, install-ci-test, install-test, link,
     ll, login, logout, ls, org, outdated, owner, pack, ping,
     pkg, prefix, profile, prune, publish, query, rebuild, repo,
-    restart, root, run-script, search, set, set-script,
-    shrinkwrap, star, stars, start, stop, team, test, token,
-    uninstall, unpublish, unstar, update, version, view, whoami
+    restart, root, run-script, search, set, shrinkwrap, star,
+    stars, start, stop, team, test, token, uninstall, unpublish,
+    unstar, update, version, view, whoami
 
 Specify configs in the ini-formatted file:
     {CWD}/smoke-tests/test/tap-testdir-index/.npmrc
@@ -597,21 +597,7 @@ exports[`test/index.js TAP npm pkg > should print package.json contents 1`] = `
 
 `
 
-exports[`test/index.js TAP npm prefix > should have expected prefix output 1`] = `
-{CWD}/smoke-tests/test/tap-testdir-index/project
-
-`
-
-exports[`test/index.js TAP npm run-script > should have expected run-script output 1`] = `
-
-> project@1.0.0 hello
-> echo Hello
-
-Hello
-
-`
-
-exports[`test/index.js TAP npm set-script > should have expected script added package.json result 1`] = `
+exports[`test/index.js TAP npm pkg set scripts > should have expected script added package.json result 1`] = `
 {
   "name": "project",
   "version": "1.0.0",
@@ -634,7 +620,21 @@ exports[`test/index.js TAP npm set-script > should have expected script added pa
 
 `
 
-exports[`test/index.js TAP npm set-script > should have expected set-script output 1`] = `
+exports[`test/index.js TAP npm pkg set scripts > should have expected set-script output 1`] = `
+
+`
+
+exports[`test/index.js TAP npm prefix > should have expected prefix output 1`] = `
+{CWD}/smoke-tests/test/tap-testdir-index/project
+
+`
+
+exports[`test/index.js TAP npm run-script > should have expected run-script output 1`] = `
+
+> project@1.0.0 hello
+> echo Hello
+
+Hello
 
 `
 
diff --git a/smoke-tests/test/index.js b/smoke-tests/test/index.js
index bbb833a5728c8..d9a2a8833884e 100644
--- a/smoke-tests/test/index.js
+++ b/smoke-tests/test/index.js
@@ -224,8 +224,8 @@ t.test('npm outdated', async t => {
   t.matchSnapshot(err.stdout, 'should have expected outdated output')
 })
 
-t.test('npm set-script', async t => {
-  const cmdRes = await exec('set-script', 'hello', 'echo Hello')
+t.test('npm pkg set scripts', async t => {
+  const cmdRes = await exec('pkg', 'set', 'scripts.hello=echo Hello')
 
   t.matchSnapshot(cmdRes, 'should have expected set-script output')
   t.matchSnapshot(
diff --git a/tap-snapshots/test/lib/commands/completion.js.test.cjs b/tap-snapshots/test/lib/commands/completion.js.test.cjs
index 6bd7901e205fb..fb4c53a0205fb 100644
--- a/tap-snapshots/test/lib/commands/completion.js.test.cjs
+++ b/tap-snapshots/test/lib/commands/completion.js.test.cjs
@@ -94,7 +94,6 @@ Array [
       run-script
       search
       set
-      set-script
       shrinkwrap
       star
       stars
diff --git a/tap-snapshots/test/lib/load-all-commands.js.test.cjs b/tap-snapshots/test/lib/load-all-commands.js.test.cjs
index ad7acd359b984..251aa0ec6ff20 100644
--- a/tap-snapshots/test/lib/load-all-commands.js.test.cjs
+++ b/tap-snapshots/test/lib/load-all-commands.js.test.cjs
@@ -790,19 +790,6 @@ npm set <key>=<value> [<key>=<value> ...] (See \`npm config\`)
 Run "npm help set" for more info
 `
 
-exports[`test/lib/load-all-commands.js TAP load each command set-script > must match snapshot 1`] = `
-Set tasks in the scripts section of package.json, deprecated
-
-Usage:
-npm set-script [<script>] [<command>]
-
-Options:
-[-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
-[-ws|--workspaces] [--include-workspace-root]
-
-Run "npm help set-script" for more info
-`
-
 exports[`test/lib/load-all-commands.js TAP load each command shrinkwrap > must match snapshot 1`] = `
 Lock down dependency versions for publication
 
diff --git a/tap-snapshots/test/lib/npm.js.test.cjs b/tap-snapshots/test/lib/npm.js.test.cjs
index a728654a51d43..68adb55516c72 100644
--- a/tap-snapshots/test/lib/npm.js.test.cjs
+++ b/tap-snapshots/test/lib/npm.js.test.cjs
@@ -27,9 +27,9 @@ All commands:
     hook, init, install, install-ci-test, install-test, link,
     ll, login, logout, ls, org, outdated, owner, pack, ping,
     pkg, prefix, profile, prune, publish, query, rebuild, repo,
-    restart, root, run-script, search, set, set-script,
-    shrinkwrap, star, stars, start, stop, team, test, token,
-    uninstall, unpublish, unstar, update, version, view, whoami
+    restart, root, run-script, search, set, shrinkwrap, star,
+    stars, start, stop, team, test, token, uninstall, unpublish,
+    unstar, update, version, view, whoami
 
 Specify configs in the ini-formatted file:
     /some/config/file/.npmrc
@@ -63,9 +63,9 @@ All commands:
     hook, init, install, install-ci-test, install-test, link,
     ll, login, logout, ls, org, outdated, owner, pack, ping,
     pkg, prefix, profile, prune, publish, query, rebuild, repo,
-    restart, root, run-script, search, set, set-script,
-    shrinkwrap, star, stars, start, stop, team, test, token,
-    uninstall, unpublish, unstar, update, version, view, whoami
+    restart, root, run-script, search, set, shrinkwrap, star,
+    stars, start, stop, team, test, token, uninstall, unpublish,
+    unstar, update, version, view, whoami
 
 Specify configs in the ini-formatted file:
     /some/config/file/.npmrc
@@ -99,9 +99,9 @@ All commands:
     hook, init, install, install-ci-test, install-test, link,
     ll, login, logout, ls, org, outdated, owner, pack, ping,
     pkg, prefix, profile, prune, publish, query, rebuild, repo,
-    restart, root, run-script, search, set, set-script,
-    shrinkwrap, star, stars, start, stop, team, test, token,
-    uninstall, unpublish, unstar, update, version, view, whoami
+    restart, root, run-script, search, set, shrinkwrap, star,
+    stars, start, stop, team, test, token, uninstall, unpublish,
+    unstar, update, version, view, whoami
 
 Specify configs in the ini-formatted file:
     /some/config/file/.npmrc
@@ -135,9 +135,9 @@ All commands:
     hook, init, install, install-ci-test, install-test, link,
     ll, login, logout, ls, org, outdated, owner, pack, ping,
     pkg, prefix, profile, prune, publish, query, rebuild, repo,
-    restart, root, run-script, search, set, set-script,
-    shrinkwrap, star, stars, start, stop, team, test, token,
-    uninstall, unpublish, unstar, update, version, view, whoami
+    restart, root, run-script, search, set, shrinkwrap, star,
+    stars, start, stop, team, test, token, uninstall, unpublish,
+    unstar, update, version, view, whoami
 
 Specify configs in the ini-formatted file:
     /some/config/file/.npmrc
@@ -829,17 +829,6 @@ All commands:
                     
                     Run "npm help set" for more info
 
-    set-script      Set tasks in the scripts section of package.json, deprecated
-                    
-                    Usage:
-                    npm set-script [<script>] [<command>]
-                    
-                    Options:
-                    [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
-                    [-ws|--workspaces] [--include-workspace-root]
-                    
-                    Run "npm help set-script" for more info
-
     shrinkwrap      Lock down dependency versions for publication
                     
                     Usage:
diff --git a/tap-snapshots/test/lib/utils/cmd-list.js.test.cjs b/tap-snapshots/test/lib/utils/cmd-list.js.test.cjs
index e88cf67fa54e7..062f9fefd273b 100644
--- a/tap-snapshots/test/lib/utils/cmd-list.js.test.cjs
+++ b/tap-snapshots/test/lib/utils/cmd-list.js.test.cjs
@@ -260,13 +260,6 @@ Object {
     "searc": "search",
     "search": "search",
     "set": "set",
-    "set-": "set-script",
-    "set-s": "set-script",
-    "set-sc": "set-script",
-    "set-scr": "set-script",
-    "set-scri": "set-script",
-    "set-scrip": "set-script",
-    "set-script": "set-script",
     "sho": "show",
     "show": "show",
     "shr": "shrinkwrap",
@@ -464,7 +457,6 @@ Object {
     "run-script",
     "search",
     "set",
-    "set-script",
     "shrinkwrap",
     "star",
     "stars",
diff --git a/test/lib/commands/set-script.js b/test/lib/commands/set-script.js
deleted file mode 100644
index cf0df53af14bb..0000000000000
--- a/test/lib/commands/set-script.js
+++ /dev/null
@@ -1,188 +0,0 @@
-const t = require('tap')
-const fs = require('fs')
-const parseJSON = require('json-parse-even-better-errors')
-const { fake: mockNpm } = require('../../fixtures/mock-npm')
-const { resolve } = require('path')
-
-const flatOptions = {}
-const npm = mockNpm(flatOptions)
-
-const ERROR_OUTPUT = []
-const WARN_OUTPUT = []
-const SetScript = t.mock('../../../lib/commands/set-script.js', {
-  'proc-log': {
-    error: (...args) => {
-      ERROR_OUTPUT.push(args)
-    },
-    warn: (...args) => {
-      WARN_OUTPUT.push(args)
-    },
-  },
-})
-const setScript = new SetScript(npm)
-
-t.test('completion', t => {
-  t.test('already have a script name', async t => {
-    npm.localPrefix = t.testdir({})
-    const res = await setScript.completion({ conf: { argv: { remain: ['npm', 'run', 'x'] } } })
-    t.equal(res, undefined)
-    t.end()
-  })
-
-  t.test('no package.json', async t => {
-    npm.localPrefix = t.testdir({})
-    const res = await setScript.completion({ conf: { argv: { remain: ['npm', 'run'] } } })
-    t.strictSame(res, [])
-    t.end()
-  })
-
-  t.test('has package.json, no scripts', async t => {
-    npm.localPrefix = t.testdir({
-      'package.json': JSON.stringify({}),
-    })
-    const res = await setScript.completion({ conf: { argv: { remain: ['npm', 'run'] } } })
-    t.strictSame(res, [])
-    t.end()
-  })
-
-  t.test('has package.json, with scripts', async t => {
-    npm.localPrefix = t.testdir({
-      'package.json': JSON.stringify({
-        scripts: { hello: 'echo hello', world: 'echo world' },
-      }),
-    })
-    const res = await setScript.completion({ conf: { argv: { remain: ['npm', 'run'] } } })
-    t.strictSame(res, ['hello', 'world'])
-    t.end()
-  })
-
-  t.end()
-})
-
-t.test('fails on invalid arguments', async t => {
-  t.plan(3)
-  await t.rejects(
-    setScript.exec(['arg1']),
-    /Expected 2 arguments: got 1/
-  )
-  await t.rejects(
-    setScript.exec(['arg1', 'arg2', 'arg3']),
-    /Expected 2 arguments: got 3/
-  )
-  await t.rejects(
-    setScript.exec(['arg1', 'arg2', 'arg3', 'arg4']),
-    /Expected 2 arguments: got 4/
-  )
-})
-
-t.test('fails if run in postinstall script', async t => {
-  const lifecycleEvent = process.env.npm_lifecycle_event
-  t.teardown(() => {
-    process.env.npm_lifecycle_event = lifecycleEvent
-  })
-
-  process.env.npm_lifecycle_event = 'postinstall'
-  t.plan(1)
-  await t.rejects(
-    setScript.exec(['arg1', 'arg2']),
-    /Scripts can’t set from the postinstall script/
-  )
-})
-
-t.test('fails when package.json not found', async t => {
-  t.plan(1)
-  await t.rejects(
-    setScript.exec(['arg1', 'arg2']),
-    /package.json not found/
-  )
-})
-
-t.test('fails on invalid JSON', async t => {
-  npm.localPrefix = t.testdir({
-    'package.json': 'iamnotjson',
-  })
-
-  t.plan(1)
-  await t.rejects(
-    setScript.exec(['arg1', 'arg2']),
-    /Invalid package.json: JSONParseError/
-  )
-})
-
-t.test('creates scripts object', async t => {
-  npm.localPrefix = t.testdir({
-    'package.json': '{}',
-  })
-
-  await setScript.exec(['arg1', 'arg2'])
-  const contents = fs.readFileSync(resolve(npm.localPrefix, 'package.json'))
-  t.ok(parseJSON(contents), { scripts: { arg1: 'arg2' } })
-})
-
-t.test('warns when overwriting', async t => {
-  WARN_OUTPUT.length = 0
-  npm.localPrefix = t.testdir({
-    'package.json': JSON.stringify({
-      scripts: {
-        arg1: 'blah',
-      },
-    }),
-  })
-
-  await setScript.exec(['arg1', 'arg2'])
-  t.hasStrict(WARN_OUTPUT[1], ['set-script', 'Script "arg1" was overwritten'], 'warning was logged')
-})
-
-t.test('workspaces', async t => {
-  ERROR_OUTPUT.length = 0
-  WARN_OUTPUT.length = 0
-  npm.localPrefix = t.testdir({
-    'package.json': JSON.stringify({
-      name: 'workspaces-test',
-      version: '1.0.0',
-      workspaces: ['workspace-a', 'workspace-b', 'workspace-c'],
-    }),
-    'workspace-a': {
-      'package.json': '{}',
-    },
-    'workspace-b': {
-      'package.json': '"notajsonobject"',
-    },
-    'workspace-c': {
-      'package.json': JSON.stringify({
-        scripts: {
-          arg1: 'test',
-        },
-      }, null, ' '.repeat(6)).replace(/\n/g, '\r\n'),
-    },
-  })
-
-  await setScript.execWorkspaces(['arg1', 'arg2'], [])
-  t.equal(process.exitCode, 1, 'did set the exitCode to 1')
-  // force the exitCode back to 0 to make tap happy
-  process.exitCode = 0
-
-  // workspace-a had the script added
-  const contentsA = fs.readFileSync(resolve(npm.localPrefix, 'workspace-a', 'package.json'))
-  const dataA = parseJSON(contentsA)
-  t.hasStrict(dataA, { scripts: { arg1: 'arg2' } }, 'defined the script')
-
-  // workspace-b logged an error
-  t.strictSame(ERROR_OUTPUT, [
-    ['set-script', `Can't update invalid package.json data`],
-    ['  in workspace: workspace-b'],
-    [`  at location: ${resolve(npm.localPrefix, 'workspace-b')}`],
-  ], 'logged workspace-b error')
-
-  // workspace-c overwrite a script and logged a warning
-  const contentsC = fs.readFileSync(resolve(npm.localPrefix, 'workspace-c', 'package.json'))
-  const dataC = parseJSON(contentsC)
-  t.hasStrict(dataC, { scripts: { arg1: 'arg2' } }, 'defined the script')
-  t.equal(dataC[Symbol.for('indent')], ' '.repeat(6), 'kept the correct indent')
-  t.equal(dataC[Symbol.for('newline')], '\r\n', 'kept the correct newline')
-  t.match(WARN_OUTPUT, [
-    ['set-script', 'Script "arg1" was overwritten'],
-    ['  in workspace: workspace-c'],
-    [`  at location: ${resolve(npm.localPrefix, 'workspace-c')}`],
-  ], 'logged workspace-c warning')
-})