Skip to content

feat(@angular/cli): prompt to set up autocompletion and add ng completion init #23060

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

Merged
merged 5 commits into from
May 3, 2022

Conversation

dgp1130
Copy link
Collaborator

@dgp1130 dgp1130 commented Apr 29, 2022

This updates the CLI to set up autocompletion for users automatically. Running ng completion now finds the relevant ~/.bashrc, ~/.zshrc, etc. file and adds the command to load Angular autocompletion for you, so users don't have to understand how to do it. ng completion script prints the actual shell script which needs to be evaluated in the shell (previously this was done by ng completion directly).

This also includes a prompt which is displayed to users on first command execution. After set up or if the user refuses the prompt, the CLI stores this state in the global workspace configuration and avoids prompting again. Users are expected to use ng completion if they want to set up autocompletion after previously refusing the prompt. This hopefully provides a good middle ground where users who might not be aware of the feature can discover it without being too annoying to users who have custom shell setups or don't want autocompletion.

The prompt is also skipped when running on CI, in a non-interactive terminal, when running ng completion or ng update commands, or if a run configuration for the current shell already has ng completion script in it (implying the user already set this up).

End-to-end tests cover most of the flows, though there are a couple error conditions which are impractical to test. This required some modifications to test infrastructure to support passing stdin and environment variables in the right places. I also had to rewrite analytics tests to be more stable and not break from adding a new prompt.

Closes #23003.

@dgp1130 dgp1130 added the target: major This PR is targeted for the next major release label Apr 29, 2022
@dgp1130 dgp1130 added this to the v14 milestone Apr 29, 2022
@dgp1130 dgp1130 requested a review from clydin April 29, 2022 23:35
@dgp1130
Copy link
Collaborator Author

dgp1130 commented Apr 29, 2022

/cc @twerske

@dgp1130 dgp1130 force-pushed the auto-completion branch 4 times, most recently from 9298e4f to 42ca16c Compare April 30, 2022 01:52
`Invalid config found at ${wksp.filePath}. \`extensions.cli\` should be an object.`,
);
}
cli.completion ??= config as json.JsonObject;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to be in the JSON schema.

This needs to be added

const validGlobalCliPaths = new Set<string>([
and also in the workspace schema and https://github.com/angular/angular-cli/blob/main/packages/angular/cli/lib/config/workspace-schema.json

Note: at the moment options can be both at global and local level, currently there is no schema for global configs only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out, added cli.completion.prompted (I'm not sure if we expect cli.completion to do anything)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also wondering if we should have a more centralized workspace definition with some runtime component so we don't need to hard-code these paths? It would be very easy to forget them in the future.

}

run(): void {
yargs.showCompletionScript();
}
}

class CompletionInitCommandModule extends CommandModule implements CommandModuleImplementation {
command = 'init';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of having a sub command, the prompt should be in the completion command?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see ng completion as responsible for generating the shell configuration for auto completion, while ng completion init is for setting up your .bashrc to use ng completion. Since these are two different actions, I think they should be different commands.

Considering that ng completion is something that users won't generally be calling themselves, I could see changing ng completion to initialize the setup by appending source <(ng completion --get-shell-config) or something if we want to view that part as more of an implementation detail. I could go either way whether --get-shell-config should be a flag on ng completion or a subcommand.

Copy link
Collaborator

@alan-agius4 alan-agius4 May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the get-shell-config concept. Maybe since users will not need to use this command/option it can also be hidden both from help and docs (aio).

Nit: maybe get-completion-script would be a better name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like ng completion being the one that users run to set up autocompletion and the ng completion some-flag-or-command being the one that is added to shell configs, since users care less about that. I'm don't feel very strongly about some-flag-or-command given that it still needs to be public API anyways just for people who customize their .bashrc setups heavily. I think I still slightly prefer a subcommand rather than a flag since it has a completely different use case and different outputs which would be a lot to change with a single flag. As for the subcommand name get-completion-script is a little long and redundant, but it's also not something that users regularly type, so we don't need to overthink it too much.

I guess where I'm landing is:

  • ng completion modifies your .bashrc to set up autocompletion for you.
  • ng completion get-completion-script prints the Yargs setup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clydin suggested ng completion script, which I like since we don't need to repeat "completion" (and the "get" isn't that valuable either).

@alan-agius4
Copy link
Collaborator

Using <(ng completion) will slow down the terminal spawning by around 300ms (The time need for ng completion to run).

Maybe we should consider adding directly the script instead?

@dgp1130 dgp1130 force-pushed the auto-completion branch 2 times, most recently from 4b795b1 to 9777d75 Compare May 2, 2022 22:33
@dgp1130
Copy link
Collaborator Author

dgp1130 commented May 2, 2022

Using <(ng completion) will slow down the terminal spawning by around 300ms (The time need for ng completion to run).

Maybe we should consider adding directly the script instead?

I was wondering about this too. My thought process was that having users inline Yargs output would be leaking an implementation detail. It becomes harder to change in the future (whether by us or Yargs itself), is less clear about its purpose, and harder to detect if it is already setup (need to search for Yargs output which more tightly couples us to it).

Where did you get the 300ms number from? Is that some general understanding you have about the CLI startup time or something you've directly measured?

@dgp1130 dgp1130 force-pushed the auto-completion branch from a71fd8f to e476015 Compare May 2, 2022 22:56
@dgp1130 dgp1130 requested a review from alan-agius4 May 3, 2022 01:15
@alan-agius4
Copy link
Collaborator

Using <(ng completion) will slow down the terminal spawning by around 300ms (The time need for ng completion to run).
Maybe we should consider adding directly the script instead?

I was wondering about this too. My thought process was that having users inline Yargs output would be leaking an implementation detail. It becomes harder to change in the future (whether by us or Yargs itself), is less clear about its purpose, and harder to detect if it is already setup (need to search for Yargs output which more tightly couples us to it).

Where did you get the 300ms number from? Is that some general understanding you have about the CLI startup time or something you've directly measured?

This is something I measured, and is the time taken to execute the ng completion command.

@dgp1130 dgp1130 force-pushed the auto-completion branch from e476015 to 865d777 Compare May 3, 2022 19:44
This fixes a few issues with the test:
1. The test accidentally used the real user's `$HOME` directory which led to confusing, non-hermetic failures.
2. The test had timeouts of 5 milliseconds, rather than the presumably intended 5 seconds.
3. `ng update` would always fail because there's no project. Changed to `ng update --help` which still skips the analytics prompt but doesn't require a complicated setup.
4. The test would end after seeing the analytics prompt and wouldn't bother accepting it. I added functionality to send data to stdin to accept the prompt which makes test logic simpler (no need to manually kill all processes or wait for a timeout), though I didn't add any new assertions that the CLI actually tracks / doesn't track correctly.

Refs angular#23003.
@dgp1130 dgp1130 force-pushed the auto-completion branch from 865d777 to 4b12d86 Compare May 3, 2022 19:47
@dgp1130
Copy link
Collaborator Author

dgp1130 commented May 3, 2022

This is something I measured, and is the time taken to execute the ng completion command.

Hmm, 300ms startup isn't great, but I'm also wary of exposing that shell configuration too broadly. I'm thinking we start with source <(ng completion) and see how people react. We can consider other improvements to startup time where possible and reevaluate if there's significant push back, we can always change it later (though updating existing .bashrc files might be tricky).

I'm also thinking that power users are likely willing to inline ng completion output even if it's not strict public API, they could do that now if they want. It could change in a patch version, but that is 1) probably uncommon and 2) easy to fix when it happens. I imagine some folks will do that and it's probably fine, just not something we want to recommend unless they understand the consequences of that choice.

dgp1130 added 2 commits May 3, 2022 13:19
… modifying `.bashrc` files

`ng completion` is changed to set up Angular CLI autocompletion for the current user by appending `source <(ng completion script)` to their `~/.bashrc`, `~/.bash_profile`, `~/.zshrc`, `~/.zsh_profile`, or `~/.profile`.

The previous `ng completion` functionality (printing Yargs autocompletion shell script) is moved to `ng completion script` because most users won't need to worry about this, so we're prioritizing `ng completion` as the part most users will actually type.

I couldn't find a good way of testing an error when writing to the `~/.bashrc` file. Since the CLI checks if it has access to the file first, that would usually fail in any circumstance when the file can't be written to. Things could change in between (user modifies file permissions or disk runs out of storage), but there's no easy hook to simulate this change in the e2e test.

Refs angular#23003.
When the CLI is executed with any command, it will check if `ng completion script` is already included in the user's `~/.bashrc` file (or similar) and if not, ask the user if they would like it to be configured for them. The CLI checks any existing `~/.bashrc`, `~/.zshrc`, `~/.bash_profile`, `~/.zsh_profile`, and `~/.profile` files for `ng completion script`, and if that string is found for the current shell's configuration files, this prompt is skipped. If the user refuses the prompt, no action is taken and the CLI continues on the command the user originally requested.

Refs angular#23003.
dgp1130 added 2 commits May 3, 2022 13:36
…letion and don't prompt again

After the user rejects the autocompletion prompt or accepts and is successfully configured, the state is saved into the Angular CLI's global configuration. Before displaying the autocompletion prompt, this state is checked and the prompt is skipped if it was already shown. If the user accepts the prompt but the setup process fails, then the CLI will prompt again on the next execution, this gives users an opportunity to fix whatever issue they are encountering and try again.

Refs angular#23003.
…ate` and `ng completion` commands

`ng update` is most likely called when upgrading a project to the next version and users should be more concerned about their project than their personal terminal setup.

`ng completion` is unconditionally setting up autocompletion, while `ng completion script` is getting the shell script for autocompletion setup. As a result, both of these don't benefit from a prompt and should be safe to skip it.
@dgp1130 dgp1130 force-pushed the auto-completion branch from 4b12d86 to 1082963 Compare May 3, 2022 20:39
@dgp1130 dgp1130 removed the request for review from alan-agius4 May 3, 2022 21:37
@dgp1130 dgp1130 added the action: merge The PR is ready for merge by the caretaker label May 3, 2022
@dgp1130 dgp1130 merged commit fb06228 into angular:main May 3, 2022
@dgp1130 dgp1130 deleted the auto-completion branch May 3, 2022 21:39
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a default prompt to run ng completion
3 participants