fix(cli): minimist should not turn strings into numbers - #39148
Conversation
| Object.entries(commands).map(([name, command]) => [name, generateCommandHelp(command)]) | ||
| ), | ||
| stringOptions: [...stringOptions], | ||
| booleanOptions: [...booleanOptions], |
There was a problem hiding this comment.
this outputs a lot more booleanOptions than we currently have:
"stringOptions": [
"browser",
"config",
"profile",
"modifiers",
"filename",
"domain",
"path",
"body",
"content-type",
"remove-header"
],
"booleanOptions": [
"extension",
"headed",
"persistent",
"submit",
"clear",
"full-page",
"httpOnly",
"secure",
"skills",
"static"
]| const argv = process.argv.slice(2); | ||
| const args: MinimistArgs = require('minimist')(argv, { boolean: booleanOptions }); | ||
| for (const option of booleanOptions) { | ||
| const args: MinimistArgs = require('minimist')(argv, { boolean: [...help.booleanOptions, 'help', 'version'], string: [...help.stringOptions, '_'] }); |
There was a problem hiding this comment.
Does this work with mousemove? IIRC, it has two number arguments.
There was a problem hiding this comment.
CI says no!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| const argv = process.argv.slice(2); | ||
| const args: MinimistArgs = require('minimist')(argv, { boolean: booleanOptions }); | ||
| for (const option of booleanOptions) { | ||
| const args: MinimistArgs = require('minimist')(argv, { boolean: [...help.booleanOptions, 'help', 'version'], string: [...help.stringOptions, '_'] }); |
There was a problem hiding this comment.
I think we should still include booleanOptions and globalOptions that are cli-only, instead of relying upon them being listed in help.
Alternatively, we can expose those from help, but I'd prefer that to be a separate PR.
| const { error, exitCode } = await cli('type', 'foo', '--submit=bar'); | ||
| expect(exitCode).toBe(1); | ||
| expect(error).toContain(`error: '--submit' option: expected boolean, received string`); | ||
| // boolean options are silently allowed |
There was a problem hiding this comment.
- Why is that? I'd prefer this to throw.
- I also noticed that
type --submit foodoes not work on ToT. I hope it works with this PR, let's add a test for that.
| expect(snapshot).toContain(`- textbox [ref=e2]`); | ||
|
|
||
| const { snapshot: fillSnapshot } = await cli('fill', 'e2', '42', '--submit'); | ||
| expect(fillSnapshot).toBe(`- textbox [active] [ref=e2]: "42"`); |
There was a problem hiding this comment.
The [active] part will fail on some webkit bots.
Test results for "MCP"4 failed 4723 passed, 135 skipped Merge workflow run. |
2 participants
Closes microsoft/playwright-cli#235