-
Notifications
You must be signed in to change notification settings - Fork 12k
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
Conversation
e7b8f72
to
70fc62a
Compare
/cc @twerske |
9298e4f
to
42ca16c
Compare
`Invalid config found at ${wksp.filePath}. \`extensions.cli\` should be an object.`, | ||
); | ||
} | ||
cli.completion ??= config as json.JsonObject; |
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.
This doesn't appear to be in the JSON schema.
This needs to be added
const validGlobalCliPaths = new Set<string>([ |
Note: at the moment options can be both at global and local level, currently there is no schema for global configs only.
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.
Thanks for pointing that out, added cli.completion.prompted
(I'm not sure if we expect cli.completion
to do anything)?
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.
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'; |
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.
Maybe instead of having a sub command, the prompt should be in the completion
command?
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.
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.
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.
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.
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.
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.
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.
@clydin suggested ng completion script
, which I like since we don't need to repeat "completion" (and the "get" isn't that valuable either).
Using Maybe we should consider adding directly the script instead? |
4b795b1
to
9777d75
Compare
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 |
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.
Hmm, 300ms startup isn't great, but I'm also wary of exposing that shell configuration too broadly. I'm thinking we start with I'm also thinking that power users are likely willing to inline |
… 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.
…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.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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 byng 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
orng update
commands, or if a run configuration for the current shell already hasng 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.