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

feat: Add a script to set up the PATH and install shell completions #295

Merged
merged 29 commits into from
Sep 24, 2024

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Sep 18, 2024

Ref denoland/deno#24157

Written in typescript for maintainability and ease of contribution (I am not good at bash scripts, and I think most people are more comfortable with TS than bash). The downside is that we only support this for deno 1.42 and above. I think this is fine because most people installing deno interactively are likely to be installing new deno versions.

The actual PATH setup logic and general shape is largely adapted from rustup.

Here's what it looks like currently:

Screen.Recording.2024-09-17.at.6.51.06.PM.mov

Some features:

  • Supports sh, bash, zsh, and fish
  • Set up PATH vars for supported shells
  • Optional completion setup for supported shells
  • Backs up rc files in case you want to revert

Things I am unsure about:

  • Whether to prompt for writing to outside of the install dir
    • currently I only allow-write for the install dir (~/.deno)
    • I thought it was a nice opportunity to show of the permission system, and so users know what we're modifying, but it may be more noise than it's worth
  • Whether bash completions work (we recommend installing to /usr/local/etc, but that requires root)

@bartlomieju
Copy link
Member

Whether to prompt for writing to outside of the install dir
currently I only allow-write for the install dir (~/.deno)
I thought it was a nice opportunity to show of the permission system, and so users know what we're modifying, but it may be more noise than it's worth

That seems undesirable IMO - running installation script shouldn't prompt at all - we should add entry to .bashrc by default and print information about it. Power users who don't want that we'll be able to remove it.

@@ -0,0 +1,9 @@
MIT License
Copy link
Member

Choose a reason for hiding this comment

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

lol

@littledivy
Copy link
Member

That seems undesirable IMO - running installation script shouldn't prompt at all

I agree. The shell script writes out deno binary to file system anyways so asking for permission doesn't make sense.

@lucacasonato
Copy link
Member

It would be nice if we bundled the code here into a single JS file and ran that. Pulling in dax in the installer is kinda annoying.

install.sh Outdated Show resolved Hide resolved
@nathanwhit
Copy link
Member Author

Okay, I updated this to bundle into one entrypoint, and also removed the dependency on dax (I was just using it for prompts, and it's overkill. Instead I just adapted some of the code from dax, replacing the rust WASM dependencies with js, and published a little library https://jsr.io/@nathanwhit/promptly). That brings the bundled size down to 20KB.

Also updated to avoid permission prompts, as suggested.


All that's left is to publish to JSR, and then this should be good to go.

@nathanwhit
Copy link
Member Author

Published to JSR, this should be ready to go.

Just need to merge this PR and update deno.land/install.sh

install.sh Outdated Show resolved Hide resolved
shell-setup/src/environment.ts Show resolved Hide resolved
Comment on lines +284 to +287
const selected = await multiSelect(
{
message: `Set up completions?`,
options: shellsWithCompletion.map((s) => {
Copy link
Member

Choose a reason for hiding this comment

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

So this is an interactive prompt - are we sure we want to prompt user and require more interaction? I'm fine either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I'd prefer to have it opt-in, since setting up completions is more invasive that adding to path, and more dependent on your shell setup.

We can change it in the future if it's too annoying

@nathanwhit nathanwhit merged commit 5b78296 into denoland:master Sep 24, 2024
4 checks passed
@nathanwhit nathanwhit deleted the shell-setup branch September 24, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants