feat: run scripts without -- (#4290)

close #3778

Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/odd-kangaroos-care.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/parse-cli-args": major
+"@pnpm/plugin-commands-script-runners": major
+"pnpm": major
+---
+
+When using `pnpm run <script>`, all command line arguments after the script name are now passed to the script's argv. Previously flagged arguments (e.g. `--silent`) were intepreted as pnpm arguments unless `--` came before it.

packages/parse-cli-args/src/index.ts

@@ -59,7 +59,12 @@ export default async function parseCliArgs (
     cmd = opts.fallbackCommand!
     commandName = opts.fallbackCommand!
     inputArgv.unshift(opts.fallbackCommand!)
-  } else if (noptExploratoryResults['help']) {
+  // The run command has special casing for --help and is handled further below.
+  } else if (cmd !== 'run' && noptExploratoryResults['help']) {
+    return getParsedArgsForHelp()
+  }
+
+  function getParsedArgsForHelp () {
     return {
       argv: noptExploratoryResults.argv,
       cmd: 'help',
@@ -69,6 +74,7 @@ export default async function parseCliArgs (
       fallbackCommandUsed: false,
     }
   }
+
   const types = {
     ...opts.universalOptionsTypes,
     ...opts.getTypesByCommandName(commandName),
@@ -84,6 +90,26 @@ export default async function parseCliArgs (
     return 'add'
   }
 
+  function getEscapeArgsWithSpecialCaseForRun () {
+    if (cmd !== 'run') {
+      return opts.escapeArgs
+    }
+
+    // We'd like everything after the run script's name to be passed to the
+    // script's argv itself. For example, "pnpm run echo --test" should pass
+    // "--test" to the "echo" script. This requires determining the script's
+    // name and declaring it as the "escape arg".
+    //
+    // The name of the run script is normally the second argument (ex: pnpm
+    // run foo), but can be pushed back by recursive commands (ex: pnpm
+    // recursive run foo) or becomes the first argument when the fallback
+    // command (ex: pnpm foo) is set to 'run'.
+    const indexOfRunScriptName = 1 +
+      (recursiveCommandUsed ? 1 : 0) +
+      (fallbackCommandUsed && opts.fallbackCommand === 'run' ? -1 : 0)
+    return [noptExploratoryResults.argv.remain[indexOfRunScriptName]]
+  }
+
   const { argv, ...options } = nopt(
     {
       recursive: Boolean,
@@ -95,9 +121,15 @@ export default async function parseCliArgs (
     },
     inputArgv,
     0,
-    { escapeArgs: opts.escapeArgs }
+    { escapeArgs: getEscapeArgsWithSpecialCaseForRun() }
   )
 
+  // For the run command, it's not clear whether --help should be passed to the
+  // underlying script or invoke pnpm's help text until an additional nopt call.
+  if (cmd === 'run' && options['help']) {
+    return getParsedArgsForHelp()
+  }
+
   if (opts.renamedOptions != null) {
     for (const cliOption of Object.keys(options)) {
       if (opts.renamedOptions[cliOption]) {

packages/parse-cli-args/test/index.ts

@@ -207,13 +207,59 @@ test('any unknown command is treated as a script', async () => {
     fallbackCommand: 'run',
     getCommandLongName: () => null,
     universalOptionsTypes: { filter: [String, Array] },
-  }, ['foo', '--recursive'])
+  }, ['--recursive', 'foo'])
   expect(cmd).toBe('run')
   expect(params).toStrictEqual(['foo'])
   expect(options).toHaveProperty(['recursive'])
   expect(fallbackCommandUsed).toBeTruthy()
 })
 
+test('run script with --help before script name is help command', async () => {
+  const { cmd, params } = await parseCliArgs({
+    ...DEFAULT_OPTS,
+    fallbackCommand: 'run',
+  }, ['run', '--help', 'foo'])
+  expect(cmd).toBe('help')
+  expect(params).toStrictEqual(['run', 'foo'])
+})
+
+test.each([
+  ['foo', { params: 'foo', options: {} }],
+  ['foo --bar baz --qux', { params: 'foo --bar baz --qux', options: {} }],
+  ['-r foo', { params: 'foo', options: { recursive: true } }],
+  ['-r foo --bar baz --qux', { params: 'foo --bar baz --qux', options: { recursive: true } }],
+
+  // Edge case where option value is the script name. Fortunately nopt handles this correctly.
+  ['--test-pattern test test foo', { params: 'test foo', options: { 'test-pattern': ['test'] } }],
+
+  // Ensure even builtin flags are passed to the script.
+  ['foo -r', { params: 'foo -r', options: {} }],
+  ['foo --recursive', { params: 'foo --recursive', options: {} }],
+  ['foo -h', { params: 'foo -h', options: {} }],
+  ['foo --help', { params: 'foo --help', options: {} }],
+  ['foo --filter=bar', { params: 'foo --filter=bar', options: {} }],
+])('run script arguments are correct for: %s', async (testInput, expected) => {
+  for (const testWithCommandFallback of [true, false]) {
+    // Whether or not the leading "run" portion of the command is written
+    // shouldn't affect its arg parsing. Test both scenarios for good measure.
+    const input = [...(testWithCommandFallback ? [] : ['run']), ...testInput.split(' ')]
+
+    const { options, cmd, params, fallbackCommandUsed } = await parseCliArgs({
+      ...DEFAULT_OPTS,
+      fallbackCommand: 'run',
+      getCommandLongName: (name) => name === 'run' ? 'run' : null,
+      universalOptionsTypes: { filter: [String, Array], 'test-pattern': [String, Array] },
+    }, input)
+    expect(cmd).toBe('run')
+    expect(params).toStrictEqual(expected.params.split(' '))
+    expect(options).toStrictEqual(expected.options)
+
+    if (testWithCommandFallback) {
+      expect(fallbackCommandUsed).toBeTruthy()
+    }
+  }
+})
+
 test("don't use the fallback command if no command is present", async () => {
   const { cmd, params } = await parseCliArgs({
     ...DEFAULT_OPTS,

packages/plugin-commands-script-runners/src/run.ts

@@ -107,7 +107,7 @@ For options that may be used with `-r`, see "pnpm help recursive"',
       FILTERING,
     ],
     url: docsUrl('run'),
-    usages: ['pnpm run <command> [-- <args>...]'],
+    usages: ['pnpm run <command> [<args>...]'],
   })
 }
 
@@ -134,6 +134,11 @@ export async function handler (
 ) {
   let dir: string
   const [scriptName, ...passedThruArgs] = params
+  // For backward compatibility
+  const firstDoubleDash = passedThruArgs.findIndex((arg) => arg === '--')
+  if (firstDoubleDash !== -1) {
+    passedThruArgs.splice(firstDoubleDash, 1)
+  }
   if (opts.recursive) {
     if (scriptName || Object.keys(opts.selectedProjectsGraph).length > 1) {
       return runRecursive(params, opts)

packages/pnpm/test/run.ts

@@ -17,7 +17,7 @@ test('run -r: pass the args to the command that is specfied in the build script'
   await fs.writeFile('project/args.json', '[]', 'utf8')
   await fs.writeFile('project/recordArgs.js', RECORD_ARGS_FILE, 'utf8')
 
-  await execPnpm(['run', '-r', '--config.enable-pre-post-scripts', 'foo', 'arg', '--', '--flag=true'])
+  await execPnpm(['run', '-r', '--config.enable-pre-post-scripts', 'foo', 'arg', '--flag=true'])
 
   const { default: args } = await import(path.resolve('project/args.json'))
   expect(args).toStrictEqual([
@@ -27,6 +27,67 @@ test('run -r: pass the args to the command that is specfied in the build script'
   ])
 })
 
+test('run: pass the args to the command that is specfied in the build script', async () => {
+  prepare({
+    name: 'project',
+    scripts: {
+      foo: 'node recordArgs',
+      postfoo: 'node recordArgs',
+      prefoo: 'node recordArgs',
+    },
+  })
+  await fs.writeFile('args.json', '[]', 'utf8')
+  await fs.writeFile('recordArgs.js', RECORD_ARGS_FILE, 'utf8')
+
+  await execPnpm(['run', 'foo', 'arg', '--flag=true'])
+
+  const { default: args } = await import(path.resolve('args.json'))
+  expect(args).toStrictEqual([
+    ['arg', '--flag=true'],
+  ])
+})
+
+// Before pnpm v7, `--` was required to pass flags to a build script.
+test('run: handle -- in a backwards compatible manner', async () => {
+  prepare({
+    name: 'project',
+    scripts: {
+      foo: 'node recordArgs',
+      postfoo: 'node recordArgs',
+      prefoo: 'node recordArgs',
+    },
+  })
+  await fs.writeFile('args.json', '[]', 'utf8')
+  await fs.writeFile('recordArgs.js', RECORD_ARGS_FILE, 'utf8')
+
+  await execPnpm(['run', 'foo', 'arg', '--', '--flag=true'])
+
+  const { default: args } = await import(path.resolve('args.json'))
+  expect(args).toStrictEqual([
+    ['arg', '--flag=true'],
+  ])
+})
+
+test('run: pass -- to the build script if specified twice', async () => {
+  prepare({
+    name: 'project',
+    scripts: {
+      foo: 'node recordArgs',
+      postfoo: 'node recordArgs',
+      prefoo: 'node recordArgs',
+    },
+  })
+  await fs.writeFile('args.json', '[]', 'utf8')
+  await fs.writeFile('recordArgs.js', RECORD_ARGS_FILE, 'utf8')
+
+  await execPnpm(['run', 'foo', '--', 'arg', '--', '--flag=true'])
+
+  const { default: args } = await import(path.resolve('args.json'))
+  expect(args).toStrictEqual([
+    ['arg', '--', '--flag=true'],
+  ])
+})
+
 test('test -r: pass the args to the command that is specfied in the build script of a package.json manifest', async () => {
   preparePackages([{
     name: 'project',