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

Add CLI shortcuts #9159

Merged
merged 11 commits into from
Jan 31, 2024
Merged

Add CLI shortcuts #9159

merged 11 commits into from
Jan 31, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 22, 2023

Changes

Adds CLI shortcuts as an easter egg for the dev server. (Updated image)

image

I didn't add for the preview server because adapters can implement they're own preview servers, and I don't want to indirectly raise the bar for adapters to also implement shortcuts because of user expectations

Testing

Tested manually (as in the screenshot)

Docs

n/a. We're merging this as an easter egg, so I don't think we should document this in the next minor blog or highlight this in the docs for example.

Copy link

changeset-bot bot commented Nov 22, 2023

🦋 Changeset detected

Latest commit: 2f596f9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Nov 22, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Base automatically changed from rework-custom-logger to main December 1, 2023 08:58
@bluwy bluwy marked this pull request as ready for review December 1, 2023 09:56
@matthewp
Copy link
Contributor

matthewp commented Dec 1, 2023

What's the reasoning for the + enter? Is this a CLI shortcut convention that I'm not aware of?

@bluwy
Copy link
Member Author

bluwy commented Dec 1, 2023

The + enter convention stems from here: vitejs/vite#14342. The issue is that we can only detect keypresses line-by-line, or char-by-char.

Reading by character requires setRawMode, but that disables all the default OS shortcuts like ctrl+c, running in background, and any rare OS handling that will be very hard for us to maintain.

Reading by line is much simpler as we don't hijack the shortcuts, but it requires a enter press to create a new line. It works like cin >> my_name in C++ for example.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

That's a lovely addition!

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Want @FredKSchott to review

@natemoo-re
Copy link
Member

Okay I took a pass and cleaned up the formatting a bit. Unfortunately I'm not seeing the shortcuts logged automatically, the messages have to be manually triggered with h + enter. The server also seems to hang when I press r + enter.

I don't love that we have to parse each message to reformat them. Ideally Vite could return the resolved shortcuts (default + custom - disabled) from bindCLIShortcuts so we could format them on a single line.

Terminal with Astro dev server output and shortcuts

@bluwy
Copy link
Member Author

bluwy commented Dec 22, 2023

I don't love that we have to parse each message to reformat them. Ideally Vite could return the resolved shortcuts (default + custom - disabled) from bindCLIShortcuts so we could format them on a single line.

ICYMI there was also a discussion about it at #9159 (comment)

@Princesseuh Princesseuh added this to the 4.2.0 milestone Jan 2, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Docs approves the changeset!

Would suggest a PR adding these three commands here in docs: https://docs.astro.build/en/reference/cli-reference/#astro-dev

@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Jan 8, 2024
@matthewp
Copy link
Contributor

@bluwy can you give an update on where this PR is, in regards to the things that prevented it from coming out in 4.0? My fuzzy recollection is that we didn't merge it because:

  1. @FredKSchott wanted to tweak the UI a little bit
  2. Worry about introducing something new right before the release.

Were those the other two reasons? (2) is no longer relevant, did (1) happen?

@bluwy
Copy link
Member Author

bluwy commented Jan 12, 2024

Yeah I think no1 is the main one. Nate made some commits to the formatting recently and volunteered to take on it on standup today, so perhaps once that's settled, we can have Fred to take another look again.

Once the formatting is resolved, it should be ready.

@FredKSchott
Copy link
Member

FYI this is unblocked by me assuming that there is no change to the startup UI! Essentially, merged as an easter-egg for now.

For the startup UI changes, I'd like to pair / work 1:1 with someone to design and then add some sort of help message to startup. @matthewp you can either have someone grab time with me on my calendar, or just assign me a Linear ticket, I'd be happy with either path.

@matthewp
Copy link
Contributor

The easter egg idea was ok when it was in order to get it out before 4.0, but now we don't have that time pressure so let's do it the right way and design the UI.

@Princesseuh Princesseuh removed this from the 4.2.0 milestone Jan 15, 2024
@ematipico ematipico added this to the 4.3 milestone Jan 18, 2024
@ematipico ematipico removed this from the 4.3 milestone Jan 26, 2024
@bluwy
Copy link
Member Author

bluwy commented Jan 30, 2024

I've went ahead and cleanup the PR based on an internal discussion in team-platform. We decided to keep this as an easter egg as we want to add more helpful shortcuts, and the current list is a little short. We may also implement showing "tips" in the CLI that may reveal this feature in the future.

@sarah11918 from your approval comment, since we're keeping this as an easter egg, I think we want to hold off documenting it for now.

@ematipico ematipico added this to the 4.3 milestone Jan 30, 2024
@sarah11918
Copy link
Member

@bluwy Yup, approval still stands, and changeset looks great! 🫡

@matthewp matthewp dismissed stale reviews from FredKSchott and themself January 30, 2024 17:11

We've talked about it!

@ematipico ematipico merged commit 7d937c1 into main Jan 31, 2024
13 checks passed
@ematipico ematipico deleted the cli-shortcuts branch January 31, 2024 13:57
@astrobot-houston astrobot-houston mentioned this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants