Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[bug]: Incompatibility between parser configuration and completion #2330

Open
AgentEnder opened this issue May 8, 2023 · 5 comments · May be fixed by #2332
Open

[bug]: Incompatibility between parser configuration and completion #2330

AgentEnder opened this issue May 8, 2023 · 5 comments · May be fixed by #2332
Labels
bug

Comments

@AgentEnder
Copy link

AgentEnder commented May 8, 2023 •
edited

Yargs Parser Configuration + Completion incompatibilities

Description

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:

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.

Reproduction

Steps to reproduce

  • Run node ./index.js --get-yargs-completions and note that there is no output.
  • Update index.js by removing the strip-dashed: true option from the parser configuration.
  • Run node ./index.js --get-yargs-completions and note that the completion feature is called and the output is as expected.

Workaround and Motivation

This issue was discovered when working on implementing completion for Nx. In our case, we have a small process which runs before yargs is bootstrapped, and preprocesses some arguments. We also happen to already export our parser configuration.

For us, we can check if process.argv[2] === '--get-yargs-completions' and remove strip-dashed: true from our parser configurations. However, this is not a general solution, and it would be nice if the completion feature could be used in conjunction with the parser configuration.

Suggested Implementation

The completionKey is fed through the parser transformations so that it matches any transformations that are applied to the arguments.

Alternate Implementations

  • Provide a way to override the completionKey. This would let us, or others who notice a similar breakage, to override the key to match the transformations that are applied to the arguments.
@shadowspawn
Copy link
Member

Exposing the parser configuration has made writing yargs code more complicated! The way this is handled in yargs-parser is by always looking for the option or its aliases and ignoring the configuration options. (This time it might be worth writing an internal convenience routine to make this easier.)

Related: #2308

@shadowspawn shadowspawn added the bug label May 8, 2023
@AgentEnder
Copy link
Author

@shadowspawn if you can point me towards the desired solution I'm happy to implement, I'm not super happy with our current workaround

@shadowspawn
Copy link
Member

I have not looked deep into this, so treat with a little scepticism.

I assume the faulty code is:

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

Prior art is I wrote code to find the coerce keys like this, looking for the original key or any of its aliases in argv. For coerce I wanted to do something with each of the aliases so filtered for all the matches.

        const coerceKeyAliases = yargs.getAliases()[coerceKey] ?? [];
        const argvKeys = [coerceKey, ...coerceKeyAliases].filter(key =>
          Object.prototype.hasOwnProperty.call(argv, key)
        );

        // Skip coerce if nothing to coerce.
        if (argvKeys.length === 0) {
          return argv;
        }

In the completion case we just want to know if the key is present, so instead of .filter() it could be .some() and collapse to a boolean rather than a list. The #completion property could be null so need to cope with that too.

So I had in mind something like (typed direct into comment, not syntax checked!):

let requestCompletions = false;
if (this.#completion!.completionKey) {
  const completionKeyAliases = yargs.getAliases()[this.#completion.completionKey] ?? [];
  requestCompletions = [this.#completion.completionKey, ...completionKeyAliases].some(key =>
    Object.prototype.hasOwnProperty.call(argv, key)
  );
}

Does that make enough sense?

Side note: why not test args.indexOf(`--${this.#completion!.completionKey}`) like is used later in the same routine?

Because looking in argv is checking the post-parsed values so a more robust test than just whether an argument matched.

@shadowspawn
Copy link
Member

(And thanks for the detailed original report, complete with motivations and suggested implementation and alternatives. Excellent.)

@AgentEnder AgentEnder linked a pull request May 9, 2023 that will close this issue
Open
@AgentEnder
Copy link
Author

PR opened 🎉, thanks @shadowspawn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: parser-configuration should work well with generated completion script AgentEnder/yargs
2 participants
@AgentEnder @shadowspawn