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.

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

Should not modify user shell dotfiles #3083

Closed
alanpearce opened this issue Jan 19, 2021 · 7 comments · Fixed by #7597
Closed

Should not modify user shell dotfiles #3083

alanpearce opened this issue Jan 19, 2021 · 7 comments · Fixed by #7597
Milestone

Comments

@alanpearce
Copy link

pnpm version: 5.15

Code to reproduce the issue:

Run pnpm install-completion zsh

Expected behavior:

I get a snippet to add to .zshrc or equivalent init file (kubectl completion zsh and helm completion zsh do this)

Actual behavior:

pnpm creates/updates .zshrc
this has no effect on my system, .zshrc is located elsewhere for.. reasons

Additional information:

  • node -v prints: v12.18.4
  • Windows, macOS, or Linux?: Linux
@kwshi
Copy link

kwshi commented Apr 27, 2021

Seconded! It's intrusive and kind of annoying for a program to modify my .bashrc. It would make much more sense for one of the following:

  • output the completion script to stdout pnpm completion-script or something (kubectl, helm, stripe, starship do this)
  • generate the completion script at build-time and/or include them in source repo

Part of the problem seems to be that this behavior is inherent to tabtab, which is what pnpm uses to provide shell completions: tabtab only exposes API to imperatively install scripts into hardcoded locations but not to, say, print the scripts to stdout. This sort of intrusive behavior has spawned several problems:

@dbaynard
Copy link
Contributor

dbaynard commented Oct 14, 2021

I’ve done something hacky (in zsh)…

#!/usr/bin/env zsh

function get_completion_script() {
  export HOME=$(mktemp -d "${TMPDIR:-/tmp}/fool-tabtab-mwahaha.XXXXX")
  pnpm install-completion zsh | head -n3 | sed 's/Added/Failed to add/' >&2
  sed -n 's/^  //p' "$HOME/.config/tabtab/zsh/pnpm.zsh"
}

get_completion_script

This fakes the home directory, passes improved output of pnpm install-completion zsh to stderr, and then prints the completion script to stdout. You may need to call pnpm directly, if it isn’t on your $PATH after changing $HOME.

I found I needed to strip the if construct from the tabtab-generated file, and the sed command keeps (and deindents) only the lines I want.

Also don’t be like me; force zsh to re-cache completions…

rm ~/.zcompdump && compinit

Edit: Try this, too; I found the first version flaky.

#!/usr/bin/env zsh

function get_completion_script() {
  export HOME=$(mktemp -d "${TMPDIR:-/tmp}/fool-tabtab-mwahaha.XXXXX")
  pnpm install-completion zsh | head -n3 | sed 's/Added/Failed to add/' >&2
  echo -e "#compdef pnpm\n"
  sed -n 's/^    //p' "$HOME/.config/tabtab/zsh/pnpm.zsh"
}

get_completion_script

@gotofritz
Copy link

Agreed. It does the same with .bashrc... I don't use one. It messes up my setup. No other tool / utils I have installed on my OS X machine works like that, why does pnpm have to be different?

@zkochan
Copy link
Member

zkochan commented May 20, 2022

I don't have objections to change how this command works. I was using tabtab as a point of reference but if other tools work differently, I am totally fine to follow best practices.

@skull-squadron
Copy link

skull-squadron commented Dec 30, 2022

install-completions is not the best way to go about it because it requires making a number of invalid and fragile assumptions.

Standard behavior: {{command}} completions [{{shell_name}}] output completions to stdout.

This allows the user to decide where to place completions in their completion path, because maybe they're global/system-wide, using a completion package like bash-completions or zsh-completions, or have somewhere in their home directory. It's not convention-over-configuration-able, unfortunately, and no good will come from trying to be too clever. hab, rustup, and most every other utility follows this convention.

Utilities should never modify rc shell dotfiles unexpectedly or without permission.

And working with the completion systems rarely requires modifying init scripts. I have tuned and optimized startup scripts that use caching technology for instant, asynchronous startup and modular rc files, so I flatly refuse to add anything to ~/.zshrc. It needs to be pnpm completions zsh > ~/.zsh-completions/_pnpm where it belongs. fpath / FPATH is an array of possible completion locations. There's no way for a script to know where to make a completion file. So just don't.

Related #5771

@KSXGitHub
Copy link
Contributor

I will create a PR that add a command called generate-completion. This command will only print the content of the completion script to stdout. Where you would want to put it is your job. Is that okay?

@KSXGitHub
Copy link
Contributor

#7597

zkochan pushed a commit that referenced this issue Feb 6, 2024
* feat(completion): generate-completion

close #3083

* feat: better error message

* test: generate-completion

* feat(completion): add powershell

* chore(deps): update @pnpm/tabtab to 0.3.0

* switch to provided type declarations
* fix typings
* update tests
* update bundle scripts

* refactor: remove unnecessary `??`

* refactor: replace a type def with provided types

* chore(deps): update @pnpm/tabtab to 0.4.0

* feat(cli): rename completion command

* chore(deps): update @pnpm/tabtab to 0.4.1

* refactor: use tabtab's new features

* fix: pass shell

* chore(deps): update @pnpm/tabtab to 0.5.0

* chore(deps): update @pnpm/tabtab to 0.5.1

* fix: remove unused import

* refactor: move completion to plugins

* feat: remove `{install,uninstall}-completion`

Just `pnpm completion` is enough

* test: fix

* refactor: direct import

* refactor: move tests to next to the lib

* refactor: merge 2 packages into 1

* fix: update changeset and remove install-completion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants