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

npm create astro@latest directory creation captures Ctrl-W "cut word" shortcut as control character #6609

Closed
1 task
johnmaguire opened this issue Mar 21, 2023 · 10 comments · Fixed by #6682
Closed
1 task
Labels
- P2: nice to have Not breaking anything but nice to have (priority)

Comments

@johnmaguire
Copy link

johnmaguire commented Mar 21, 2023

What version of astro are you using?

2.1.3

Are you using an SSR adapter? If so, which one?

N/A

What package manager are you using?

npm

What operating system are you using?

Mac

What browser are you using?

N/A

Describe the Bug

  1. npm create astro@latest (v2.1.3 Launch sequence initiated.)
  2. At prompt for project directory, press Ctrl-W, Ctrl-W to delete the contents.
  3. Type ./blog followed by return key
  4. Hit return through the rest of the prompts
  5. The newly created directory contains the C-w control character.
❯ ls -l
drwxr-xr-x     3 jmaguire  staff      96 Mar 20 23:51 ???.

cd [tab] gives this suggestion: ❯ cd $'\027'$'\027'$'\027'.

From a brief perusal of the code, it seems the bug actually lies in https://github.com/withastro/cli-kit but I wasn't sure if the issue should be filed there or here.

Link to Minimal Reproducible Example

n/a

Participation

  • I am willing to submit a pull request for this issue.
@ematipico ematipico added the - P2: nice to have Not breaking anything but nice to have (priority) label Mar 21, 2023
@ematipico
Copy link
Member

@johnmaguire, what's the expected behaviour from your point of view?

@johnmaguire
Copy link
Author

johnmaguire commented Mar 21, 2023

@ematipico A directory called "blog" should be created at this level. Control codes should not make it into the path. The presence of them also creates the awkward "[Ctrl]." directory.

Ctrl-A and Ctrl-E do not seem to work (beginning / end of line) but do not create control codes in the path.

@andremralves
Copy link
Contributor

I tested Vite's and Next's CLIs to see how they prevent this issue. Vite's CLI has the same behavior (creates this weird file) and Next's displays an info message saying that the name is in a invalid pattern.

So maybe a good idea would be to check for these invalid characters and display a message. Then the user can see that something went wrong and change the project name.

@johnmaguire
Copy link
Author

johnmaguire commented Mar 27, 2023

@andremralves That seems like much better behavior, though the ideal would be to properly handle the shortcuts. Ctrl-W actually has the expected effect, it just seems to also insert control characters unexpectedly.

FWIW, these are "emacs shortcuts" which have been available in bash since... well, as long as I've used it (so pre-2005.) I don't know if they are implemented by bash/zsh/etc. or GNU readline or what. I just know that every interactive shell command I can think of off the top of my head right now supports them (I just tested zsh, fzf, weechat, vim, and cat.)

It does appear to me that there is an attempt to detect them in cli-kit and if I understand the code correctly, which I am only skimming, a way to register callbacks for these "actions". A cursory read also suggests that actions w/o callbacks will be ignored. I think this is why the other emacs shortcuts I tried do not work.

If that is the case, a simple solution is to register the C-w shortcut, which will break its functionality but at least avoid inserting erroneous control codes. I'm not familiar enough with the JS ecosystem to build and test this change.

edit: I also see that node's readline package seems to support these keybindings. I would expect them to "just work." cli-kit seemingly breaks the behavior, but I am unclear on why control codes are being inserted for unregistered actions.

@johnmaguire
Copy link
Author

johnmaguire commented Mar 27, 2023

It looks like the bug is shared between vite and astro because vite uses this project, which cli-kit prompt seems to be heavily inspired by: https://github.com/terkelg/prompts/blob/master/lib/util/action.js

@johnmaguire
Copy link
Author

I filed terkelg/prompts#394 over there, and https://github.com/terkelg/prompts/pull/330/files looks like it would indeed solve this issue by disabling the Ctrl+W shortcut entirely.

@andremralves
Copy link
Contributor

andremralves commented Mar 28, 2023

@johnmaguire Yes, I was looking into the code base and saw that the problem comes from the cli-kit too. I think that in the create-astro package the best that can be done is to add a validation.

I will see if I can do something in the cli-kit as well, but for now I think this solution that I came up with will be good for preventing the user from creating files with non-printable chars. I filed a PR, let's see if they merge it :)

@johnmaguire
Copy link
Author

johnmaguire commented Mar 28, 2023

@andremralves Thanks for filing a PR in cli-kit! I must ask though - is there a reason you didn't include this line from the original PR made to terkelg/prompts? https://github.com/terkelg/prompts/pull/330/files#diff-384dd446a9beb22e86702df030f20bf8269923635e47ab56350b2de765baaeb9R40

This would disable the C-w shortcut entirely, avoiding an annoying iteration of "Your file name was invalid, try again!" :) As it stands, the shortcut is broken, and I think it would be better to disable it.

@andremralves
Copy link
Contributor

andremralves commented Mar 28, 2023

@johnmaguire, I didn't filed a PR for the cli-kit, I filed a PR in this repo for the create-astro package just to to add a validation checking for the presence of non-printable characters in the input. I think this can be a general solution that solves this problem even for when someone copy and paste a name with these invalid characters without typing Ctrl.

But I think your solution of disabling the Ctrl characters is a good one as well, I didn't filed it in the cli-kit bacause I thought you would like to file it yourself as you had the idea.

avoiding an annoying iteration of "Your file name was invalid, try again!"

I think it wouldn't be annoying because is something rare to happen.

@johnmaguire
Copy link
Author

johnmaguire commented Mar 28, 2023

@andremralves I'm a little confused. Is this your PR? withastro/cli-kit#15

That PR is filed with cli-kit, and looks to mirror https://github.com/terkelg/prompts/pull/330/files#diff-384dd446a9beb22e86702df030f20bf8269923635e47ab56350b2de765baaeb9R40 albeit without the part that disables C-w.

(edit: I see now that this PR was made in reasons to a different issue. All the same...)

I mentioned in an earlier post, but I am not really familiar with the JS ecosystem enough to test astro with a locally modified cli-kit. If you can give me a pointer, I'm happy to test the change and then file a PR. The behavior does appear to work as expected against an example script.

avoiding an annoying iteration of "Your file name was invalid, try again!"

I think it wouldn't be annoying because is something rare to happen.

Well, this might be a matter of opinion but I think it probably happens often. The prompt is pre-filled with text, so the natural inclination for someone familiar with these shortcuts (which have been around for probably decades) is to delete the text, the quickest method of which is to use C-w. It's almost reflexive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants