-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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 CLI keyboard shortcuts #9673
Conversation
The shortcuts include: - [r] Restart the server (useful for third party config changes) - [o] Open the project in your default browser - [f] Clear the cache for optimized dependencies (then restart the server) Co-authored-by: HomyeeKing <[email protected]>
Press "h" to toggle HMR updates on/off. Useful when you hit a bug and don't want your code edits to refresh the browser (which would mean losing the error logs and app state).
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.
Looks good at a cursory glance!
Note to other reviewer: I haven't tested this PR specifically. It's possible that other changes are needed to get each shortcut working as expected with the newest Vite version.
export async function resolveBrowserUrl(server: ViteDevServer): Promise<string> { | ||
const options = server.config.server | ||
const hostname = await resolveHostname(options.host) | ||
const port = options.port || 3000 |
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.
The actual port used is not necessarily options.port
unless strict port is enabled.
But there's already an existing utility function that determines the current server URL with the correct port:
vite/packages/vite/src/node/utils.ts
Line 807 in e8b61bb
export async function resolveServerUrls( |
The results of that function are stored in server.resolvedUrls
:
vite/packages/vite/src/node/server/index.ts
Line 376 in afbb87d
server.resolvedUrls = await resolveServerUrls( |
So, you could use server.resolvedUrls.local[]
instead of resolving it here. I think you could just default to the first URL in the array.
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.
Super, thanks!
} | ||
|
||
process.stdin | ||
.on('data', onInput) |
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.
Nit: I noticed vitest listens to the keypress
event instead of data
and only sets raw mode for TTY. Should we do the same?
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 all in for uniformity!
}, | ||
{ | ||
key: 'f', | ||
name: 'force restart', |
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.
The distinction between restart
and force restart
is unclear here IMO. The flag apparently forces the optimizer to re-bundle upon restarting the server. I think that should be reflected somehow in name
here:
name: 'force restart', | |
name: 're-bundle dependencies and restart', |
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.
Sounds good to me!
@antfu @patak-dev Any opinion here?
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 we shouldn't expose this option. It is only needed if there is a bug in dependencies optimization re-bundling logic, that at this point is quite stable. I think it may end up confusing more users than helping them.
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.
For posterity, I'd also suggest changing the key
to some variation of "r" (maybe "R") if this option makes a comeback in the future.
Thanks for keeping this PR active @joelmukuthu. Looks like there is an error when running the CLI, would you check in your local clone that the test is passing? About the exposed option, I don't know if we should expose all of them. Maybe there are some discussions I missed, but |
I added this one. It's useful when you're step debugging a client-side issue and hot-reloading would cause you to lose your state. If we don't have this option, you have to stop the dev server instead. I'd be fine with opening a separate PR for this option in the future, though. |
How does this debugging setup work? Is the idea that you would like to do changes to your files and they are big enough that you want to have intermediate save points while debugging purely in the client? |
* Disable key bindings for the server by setting this to `false`. This can be | ||
* useful if you need the `process.stdin` stream for another purpose. | ||
* | ||
* @default true |
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 should default to false for programmatic usages and only default to true in CLI
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.
@antfu was this a docs-only change? Tbh I couldn't actually figure out where bindShortcuts
was defaulted to true
in the code. @aleclarson would you please point me in the right direction?
Thanks all for the reviews. I'll look into this within the next couple of days. About the options, what I gather is that I should remove the
Will do! |
This commit (6622560) should be reverted. There's no technical reason for it, and it likely causes the issue @patak-dev mentions above. +1 for |
process.stdin.setRawMode(true) | ||
} | ||
|
||
process.stdin.on('keypress', keypressHandler).setEncoding('utf8').resume() |
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.
@patak-dev @aleclarson The keys aren't detected in this changeset because of this...
The keypress
event requires readline.emitKeypressEvents(process.stdin)
beforehand.
I think the reason Vitest uses this instead of just the data
event is because its shortcuts include a prompt for changing the test pattern input, where it needs to pause keypress handling to properly receive the input. Since our shortcuts don't have any prompts, we probably don't need this. Sorry for having suggested this change in my original review.
Thanks for the reviews; ready for re-review. |
Hi everyone, is there an update on this? Is there some way I can help to get this merged or reviewed? |
I think this function should be treat as a plugin ... this is a expand of vite server. @patak-dev @antfu |
The dev server doesn't support CLI plugins, and it doesn't plan to (or at least, that's what I've inferred from previous discussions). |
emmm.. this PR wants to communicate with |
/** | ||
* Disable key bindings for the server by setting this to `false`. This can be | ||
* useful if you need the `process.stdin` stream for another purpose. | ||
* | ||
* @default true in CLI, `false` in programmatic usage | ||
*/ | ||
bindShortcuts?: boolean |
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.
Actually this is not called at all in programmatic usage. (like printURLs)
There is currently no option to disable print of server urls, I would do the same here.
If people want more complex use case, they can use the pragmatic usage.
* Listen to `process.stdin` for pre-defined keyboard shortcuts, which are | ||
* printed to the terminal by this method. |
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.
What can be useful for non standard custom usage is separating biding from help display with a little boolean flag on the method
return process.kill(process.pid, 'SIGINT') | ||
} | ||
|
||
if (input === 'h') { |
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 can be itself a shortcut. This is common for CLI to print the help option in the help itself
export interface Shortcut { | ||
key: string | ||
description: string | ||
action(server: ViteDevServer): void | Promise<void> | ||
} | ||
|
||
export const SHORTCUTS: Shortcut[] = [ |
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.
Should this be exported?
return | ||
} | ||
|
||
if (actionRunning) { |
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.
Nit: this should be the second check
*/ | ||
config.server.hmr = config.server.hmr !== true | ||
config.logger.info( | ||
colors.cyan(` hmr ${config.server.hmr ? `enabled` : `disabled`}`) |
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 need to test but I think the two whitespaces at the beginning will look strange in the middle of Vite dev server logs
Closed in favour of #11228 |
Description
This is a rebase of #6014 to resolve merge conflicts. It introduces stdin shortcuts for restarting the dev server, opening the server in browser and toggling HMR.
h
- show all shortcutsr
- restart servero
- open in browserm
- toggle HMR on/offq
- quitAdditional context
N/A
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).