New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: run scripts without -- #4290
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
Comment on lines
+107
to
+109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic here might be a bit of a hacky way to do things. We could also:
|
||
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]) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to rearrange the |
||
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, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 === '--') | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pattern matched this from the same change in pnpm/packages/plugin-commands-script-runners/src/exec.ts Lines 76 to 79 in 0355d30
pnpm v6 allowed Although perhaps we should only swallow the first
You might expect:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although the behavior above didn't change in this PR, I do worry this will come back to bite someone that thinks Happy to make any additional followup changes before the pnpm v7 release if we want to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is fine if we don't swallow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Cool. I'll open a pull request reverting this block later tonight when I'm near my computer. |
||||||||||
if (firstDoubleDash !== -1) { | ||||||||||
passedThruArgs.splice(firstDoubleDash, 1) | ||||||||||
} | ||||||||||
if (opts.recursive) { | ||||||||||
if (scriptName || Object.keys(opts.selectedProjectsGraph).length > 1) { | ||||||||||
return runRecursive(params, opts) | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these all be
major
?