fix: parser-configuration should work well with generated completion script by AgentEnder · Pull Request #2332 · yargs/yargs

AgentEnder · 2023-05-09T22:16:16Z

There is an incompatibility between the parser configuration options and the completion feature within yargs. Setting strip-dashed: true in the parser configuration causes the completion feature to fail.

This is because the completion feature relies on passing '--get-yargs-completions' verbatim to yargs. Within the Completion class, the completionKey is explicitly set to get-yargs-completions. Within the YargsInstance, we explicitly check the following condition: completionKey in argv, in this location: main/lib/yargs-factory.ts#L2054

Since the argv has been parsed already at this point, the argument is now 'getYargsCompletions' instead of 'get-yargs-completions'. This causes the completion feature to never be called.

Fixes: #2330

shadowspawn · 2023-05-10T07:35:54Z

Long story. This is not quite right, but works in simple cases. I think there are less code changes required.

The strip-dashed spelling appearing in the "aliases" is a bit of cheat, it only appears in the parse results and is not recognised on the command-line and does not match what is on the command-line.

I added some trace statements to double-check my expectations. (I have a -d, --debug boolean option in my test program.)

At this original line:

      const requestCompletions = this.#completion!.completionKey in argv;

argv is the parse result from calling yargs-parser internally, so with default options:

$ node index.js --get-yargs-completions --debug
{
  argv: {
    _: [],
    'get-yargs-completions': true,
    getYargsCompletions: true,
    debug: true,
    d: true,
    '$0': 'index.js'
  }
}

To cope with the author changing the parser configuration, we need to be flexible about what the property got called in the parse results.

Later at this point:

        const completionArgs = args.slice(
           args.indexOf(`--${this.#completion!.completionKey}`) + 1
         );

args is the original command-line args like:

$ node index.js --get-yargs-completions --debug
{ args: [ '--get-yargs-completions', '--debug' ] }

Here, we still want to check for the original unaltered this.#completion!.completionKey no matter what "alias" we found in argv. With 'strip-dashed': true the new code looks for the stripped name and fails to remove the dashed argument.

So requestCompletions should stay just a boolean, and only that initial calculation changes. I think you can just change .find() to .some() to match the semantics. We don't care which variation was found.

shadowspawn

See long comment! I think this is close, but doing too much.

shadowspawn · 2023-05-10T07:39:56Z

This is because the completion feature relies on passing '--get-yargs-completions' verbatim to yargs.

This assumption is ok. The cause of the bug you observed is that the option name got changed in the parse results by the parser configuration settings, and fooled yargs.

shadowspawn

Thanks @AgentEnder

shadowspawn · 2023-05-11T06:22:08Z

Good description with the PR, and added a test covering multiple configurations. Nice work.

AgentEnder · 2023-05-11T12:57:08Z

Thanks for the help @shadowspawn!

Once it is merged, do you know when the next release would be? I've admittedly not paid too much attention to the yargs release schedule in the past.

shadowspawn · 2023-05-13T22:50:46Z

I am guessing next release 1-2 months after last release (which was 2023-04-27).

AgentEnder · 2024-07-26T04:01:46Z

@shadowspawn apologies, just checking in as this has been approved for a year now.

Anything external contributors can do to help keep things moving?

shadowspawn · 2024-07-26T06:23:21Z

In limbo at the moment. I am not merging PR or attempting releases, as without a second active maintainer my own PRs are blocked on needing a review so I would be unable to fix problems if I broke something.

Anything external contributors can do to help keep things moving?

After mainline is building (#2393), updating PR if needed to build cleanly would be useful. Nothing in meantime.

AgentEnder · 2024-07-26T11:40:17Z

without a second active maintainer my own PRs are blocked on needing a review so I would be unable to fix problems if I broke something.

If you all are looking for another active maintainer I'd be happy to spend a few hours a week on the project, maybe we can set up a call at some point to discuss?

shadowspawn · 2024-08-16T09:04:45Z

(Thanks for the offer @AgentEnder . Nothing in the short-term. I can't add maintainers, so no point talking with me at the moment. 😆 )

bcoe · 2025-04-11T19:06:47Z

@AgentEnder bother you to rebase, I've been cleaning up a lot of old PRs in prep for a v18 of yargs.

bcoe · 2025-04-23T20:31:38Z

@AgentEnder friendly nudge on this one. If you don't have time to revisit (this admittedly very old contribution, sorry) I will rebase and merge sometime soon (but it's probably easier for you to push a change).

bcoe · 2025-04-25T20:47:50Z

@AgentEnder thank you for the contribution, it's now available at npm i yargs@next.

| datasource | package | from | to | | ---------- | ------- | ------ | ------ | | npm | yargs | 17.7.2 | 18.0.0 | ## [v18.0.0](https://github.com/yargs/yargs/blob/HEAD/CHANGELOG.md#1800-2025-05-26) ##### ⚠ BREAKING CHANGES - command names are not derived from modules passed to `command`. - singleton usage of yargs yargs.foo, yargs().argv, has been removed. - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. - yargs is now ESM first ##### Features - commandDir now works with ESM files ([#2461](yargs/yargs#2461)) ([27eec18](yargs/yargs@27eec18)) - **locale:** adds hebrew translation ([#2357](yargs/yargs#2357)) ([4266485](yargs/yargs@4266485)) - yargs is now ESM first ([d90af45](yargs/yargs@d90af45)) - **zsh:** Add default completion as fallback ([#2331](yargs/yargs#2331)) ([e02c91b](yargs/yargs@e02c91b)) ##### Bug Fixes - addDirectory do not support absolute command dir ([#2465](yargs/yargs#2465)) ([3a40a78](yargs/yargs@3a40a78)) - allows ESM modules commands to be extensible using visit option ([#2468](yargs/yargs#2468)) ([200e1aa](yargs/yargs@200e1aa)) - **browser:** fix shims so that yargs continues working in browser context ([#2457](yargs/yargs#2457)) ([4ae5f57](yargs/yargs@4ae5f57)) - **build:** address problems with typescript compilation ([#2445](yargs/yargs#2445)) ([8d72fb3](yargs/yargs@8d72fb3)) - coerce should play well with parser configuration ([#2308](yargs/yargs#2308)) ([8343c66](yargs/yargs@8343c66)) - **deps:** update dependency yargs-parser to v22 ([#2470](yargs/yargs#2470)) ([639130d](yargs/yargs@639130d)) - exit after async handler done ([#2313](yargs/yargs#2313)) ([e326cde](yargs/yargs@e326cde)) - handle spaces in bash completion ([#2452](yargs/yargs#2452)) ([83b7788](yargs/yargs@83b7788)) - parser-configuration should work well with generated completion script ([#2332](yargs/yargs#2332)) ([888db19](yargs/yargs@888db19)) - propagate Dictionary including undefined in value type ([#2393](yargs/yargs#2393)) ([2b2f7f5](yargs/yargs@2b2f7f5)) - **zsh:** completion no longer requires double tab when using autoloaded ([0dd8fe4](yargs/yargs@0dd8fe4)) ##### Code Refactoring - command names are not derived from modules passed to `command`. ([d90af45](yargs/yargs@d90af45)) - singleton usage of yargs yargs.foo, yargs().argv, has been removed. ([d90af45](yargs/yargs@d90af45)) ##### Build System - minimum node.js versions now `^20.19.0 || ^22.12.0 || >=23`. ([d90af45](yargs/yargs@d90af45))

AgentEnder changed the title ~~chore: add failing test for parser configuration and completion~~ fix: parser-configuration should work well with generated completion script May 9, 2023

shadowspawn requested changes May 10, 2023

View reviewed changes

AgentEnder force-pushed the fix/strip-dashed-completion branch from e762ac2 to 095fdd8 Compare May 10, 2023 12:32

shadowspawn approved these changes May 11, 2023

View reviewed changes

dleavitt mentioned this pull request Jun 1, 2024

fix: coerce should play well with parser configuration #2308

Merged

AgentEnder and others added 3 commits April 25, 2025 16:36

chore: add failing test for parser configuration and completion

de6a3e4

fix: completion should play well with strip-dashed

9db007e

chore: rebase

bf7bd4f

bcoe force-pushed the fix/strip-dashed-completion branch from f0ecbf7 to bf7bd4f Compare April 25, 2025 20:39

bcoe added the ci label Apr 25, 2025

bcoe merged commit 888db19 into yargs:main Apr 25, 2025
6 checks passed

github-actions bot mentioned this pull request Apr 25, 2025

chore(main): release 18.0.0 #2325

Merged

Conversation

AgentEnder commented May 9, 2023

Uh oh!

shadowspawn commented May 10, 2023

Uh oh!

shadowspawn left a comment

Choose a reason for hiding this comment

Uh oh!

shadowspawn commented May 10, 2023

Uh oh!

shadowspawn left a comment

Choose a reason for hiding this comment

Uh oh!

shadowspawn commented May 11, 2023

Uh oh!

AgentEnder commented May 11, 2023

Uh oh!

shadowspawn commented May 13, 2023

Uh oh!

AgentEnder commented Jul 26, 2024

Uh oh!

shadowspawn commented Jul 26, 2024

Uh oh!

AgentEnder commented Jul 26, 2024

Uh oh!

shadowspawn commented Aug 16, 2024

Uh oh!

bcoe commented Apr 11, 2025

Uh oh!

bcoe commented Apr 23, 2025

Uh oh!

Uh oh!

bcoe commented Apr 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants