feat: run scripts without -- #4290

gluxon · 2022-01-30T03:11:22Z

Behavior Change

There's a lot of scenarios mentioned in the original issue that make all solutions ambiguous/non-ideal. While still not perfect, I think the behavior below will cause the least amount of confusion:

Anything after the <command> portion of pnpm run <command> is passed directly to <command> itself.
Any command line flags before the <command> portion is treated as an option to pnpm run.
For backwards compatibility, the first occurrence of -- anywhere after <command> is swallowed.

For example:

This allows pnpm run --help to still show the help text for pnpm run.
While pnpm run changeset --help will show the help text of whatever command drives the changeset script.

Notes

See feat(exec): no need to use -- #3492 for a related change.
I did not address Unrecognized pnpm CLI flags are not thrown when running scripts without "run" #4094 in this issue. I'd like to handle that in a separate PR.
I noticed that @zkochan started work on this 4 months ago and cherry-picked the new test from the feat/3778 branch: 624b8a7 Hope that was okay @zkochan.
For reference on the escapeArgs option in nopt: pnpm/nopt@446cfbe

gluxon · 2022-01-30T03:12:22Z

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'])


Had to rearrange the --recursive flag here to fix the test after these changes. This was expected from the behavior change described in the pull request description.

gluxon · 2022-01-30T03:13:53Z

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

@@ -134,6 +134,11 @@ export async function handler (
 ) {
  let dir: string
  const [scriptName, ...passedThruArgs] = params
+  // For backward compatibility
+  const firstDoubleDash = passedThruArgs.findIndex((arg) => arg === '--')


I pattern matched this from the same change in pnpm exec: #3492

pnpm/packages/plugin-commands-script-runners/src/exec.ts

Lines 76 to 79 in 0355d30

// For backward compatibility

if (params[0] === '--') {

params.shift()

}

pnpm v6 allowed -- anywhere in the args portion of pnpm run <command> [..args], so this extra .findIndex logic tries to maintain full backwards compatible behavior for that.

Although perhaps we should only swallow the first -- like pnpm exec does. I could see the behavior in the scenario below being a bit surprising. This is how pnpm behaves before and after this pull request.

❯ pnpm run --silent echo a -- b a b

You might expect:

❯ pnpm run --silent echo a -- b a -- b

Although the behavior above didn't change in this PR, I do worry this will come back to bite someone that thinks -- can be dropped entirely after this change. Just a heads up @zkochan.

Happy to make any additional followup changes before the pnpm v7 release if we want to.

I think it is fine if we don't swallow -- at all. This is a breaking change in v7. So it won't be a surprise to anyone.

👍 Cool. I'll open a pull request reverting this block later tonight when I'm near my computer.

gluxon · 2022-01-30T03:21:15Z

.changeset/odd-kangaroos-care.md

+"@pnpm/parse-cli-args": major
+"@pnpm/plugin-commands-script-runners": major
+"pnpm": major


Should these all be major?

gluxon · 2022-01-30T03:23:18Z

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

+    const indexOfRunScriptName = 1 +
+      (recursiveCommandUsed ? 1 : 0) +
+      (fallbackCommandUsed && opts.fallbackCommand === 'run' ? -1 : 0)


The logic here might be a bit of a hacky way to do things. We could also:

run nopt again to find the script name to use as the escape arg. That would result in 3 nopt calls here.

add additional behavior in pnpm's nopt fork to dynamically determine whether something is an escape arg. I'm imagining a closure that gets passed the partial CLI parsing up to that point.

gluxon · 2022-01-30T03:28:55Z

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>...]'],


Should we just drop the [--] part here like pnpm --help exec does?

zkochan · 2022-01-30T15:03:25Z

An alternative (probably easier to implement) solution would be to escape everything after run.

So you could only pass options to run before the run keyword.

This would work: pnpm -r run foo.
This would not work: pnpm run -r foo.
This would also not work: pnpm -r foo.

Cherry picked by Brandon Cheng from feat/3778 branch.

gluxon · 2022-01-30T17:55:34Z

That would be much easier to implement. I had an initial approach last week that just added run to this array:

pnpm/packages/pnpm/src/parseCliArgs.ts

Line 20 in 376c304

escapeArgs: ['dlx', 'exec'],

It did end up confusing me that pnpm run --silent echo test didn't work as expected though. (So your 2nd bullet point.) Passing --silent in between run and before the script name works in Yarn for what it's worth, and I do think that behavior is better from a user experience perspective.

Are the implementation complexities reasonable given the improved UX of this approach? I personally believe so, but happy for you to make that call.

zkochan · 2022-01-30T19:38:23Z

Passing --silent in between run and before the script name works in Yarn for what it's worth, and I do think that behavior is better from a user experience perspective.

OK, if it is supported by Yarn, then we don't really have a choice. We need to support it as well.

gluxon · 2022-01-30T20:32:44Z

Thanks for taking a look so quickly!

OK, if it is supported by Yarn, then we don't really have a choice. We need to support it as well.

I am a bit surprised by this. Although in this case I do like yarn's behavior, I would hope pnpm deviates in other situations we think it can do better. I think you should feel free to make those calls @zkochan, even if individuals like me disagree. 🙂

zkochan · 2022-01-30T20:35:25Z

I would love to but then we'll have a lot of issues from users that switch from Yarn. So it is easier just to support it.

gluxon commented Jan 30, 2022

View reviewed changes

gluxon marked this pull request as ready for review January 30, 2022 03:18

gluxon requested a review from zkochan as a code owner January 30, 2022 03:18

gluxon commented Jan 30, 2022

View reviewed changes

gluxon and others added 2 commits January 30, 2022 12:50

feat: run scripts without --

a4604be

test: add additional test for running scripts without --

098bc93

Cherry picked by Brandon Cheng from feat/3778 branch.

gluxon force-pushed the v7-run-args-optional-double-dash branch from c575e75 to 098bc93 Compare January 30, 2022 17:51

zkochan approved these changes Jan 30, 2022

View reviewed changes

zkochan merged commit c35ac78 into pnpm:v7 Jan 30, 2022

zkochan added this to the v7.0 milestone Jan 30, 2022

zkochan mentioned this pull request Jan 30, 2022

Run scripts without --? #3778

Closed

1 task

gluxon deleted the v7-run-args-optional-double-dash branch January 30, 2022 20:30

gluxon mentioned this pull request Jan 31, 2022

fix: revert backwards compatible handling of -- for run #4291

Merged

gluxon mentioned this pull request Feb 15, 2022

fix: make unrecognized CLI flags show error when using fallback command #4338

Merged

nathanhammond mentioned this pull request Jun 28, 2022

Drop -- in pnpm invocation for pnpm 7+ vercel/turbo#1432

Merged

This was referenced Jul 2, 2022

chore(deps): update pnpm to v7 mrtousif/youtube-clone#18

Merged

Update dependency pnpm to v7 edward3h/stratagemizer#14

Merged

chore(deps): update pnpm to v7 thundermiracle/gatsby-remark-prismjs-copy-button#23

Merged

This was referenced Jul 10, 2022

Update pnpm to v7 esdmr/remap#5

Merged

Update pnpm to v7 buildo/bento-design-system#257

Merged

zkochan mentioned this pull request Jul 20, 2022

Pnpm Scripting Issues with Nested pnpm run #5049

Closed

pnpm locked as resolved and limited conversation to collaborators Jul 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: run scripts without -- #4290

feat: run scripts without -- #4290

gluxon commented Jan 30, 2022 •

edited

gluxon Jan 30, 2022

gluxon Jan 30, 2022 •

edited

gluxon Jan 30, 2022

zkochan Jan 30, 2022

gluxon Jan 30, 2022

gluxon Jan 30, 2022

gluxon Jan 30, 2022

gluxon Jan 30, 2022

zkochan Jan 30, 2022

zkochan commented Jan 30, 2022 •

edited

gluxon commented Jan 30, 2022 •

edited

zkochan commented Jan 30, 2022

gluxon commented Jan 30, 2022

zkochan commented Jan 30, 2022

	// For backward compatibility
	if (params[0] === '--') {
	params.shift()
	}

feat: run scripts without -- #4290

feat: run scripts without -- #4290

Conversation

gluxon commented Jan 30, 2022 • edited

Behavior Change

Notes

Choose a reason for hiding this comment

gluxon Jan 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan commented Jan 30, 2022 • edited

gluxon commented Jan 30, 2022 • edited

zkochan commented Jan 30, 2022

gluxon commented Jan 30, 2022

zkochan commented Jan 30, 2022

gluxon commented Jan 30, 2022 •

edited

gluxon Jan 30, 2022 •

edited

zkochan commented Jan 30, 2022 •

edited

gluxon commented Jan 30, 2022 •

edited