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 CLI keyboard shortcuts #11228

Merged
merged 5 commits into from
Dec 7, 2022
Merged

feat: add CLI keyboard shortcuts #11228

merged 5 commits into from
Dec 7, 2022

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented Dec 6, 2022

This is based on the work from #9673 by @HomyeeKing , @aleclarson and @joelmukuthu

Closes #9673

Few changes have been made:

  • Don't display hint by default, I think it's nice for people building on top of Vite
  • Don't display quit in default hint, I think people know how to quit a process by default
  • Fix rebind of shortcuts for external restart
  • Send SIGTERM for ctrl+c, enable nicer exit and probably better in middleware mode (not tested)
  • Restore initial hmrConfig on HMR toggle
  • Enable custom shortcuts and use it in cli for stopping dev profiler

image

@ArnaudBarre
Copy link
Member Author

cc @benmccann, @dominikg: What do you think about this? Is this ok for SvelteKit?

@benmccann
Copy link
Collaborator

SvelteKit doesn't really do anything special, but just has users call vite directly, so any change to the CLI should generally be fine as long as our Playwright tests can still start and stop processes just the same. Since Vite also uses Playwright that's probably pretty safe, but you could invoke it against the ecosystem CI if you really wanted to be sure.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 7, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
rakkas ⏹️ cancelled
sveltekit ✅ success
vite-plugin-ssr ❌ failure
vite-plugin-react ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ❌ failure
vitepress ✅ success
vitest ❌ failure
windicss ✅ success

@joelmukuthu joelmukuthu mentioned this pull request Dec 7, 2022
9 tasks
@dimshik100
Copy link

This patch appears to add keyboard shortcut support to the Vite CLI. The main changes are the addition of a new file, shortcuts.ts, which exports a bindShortcuts function that binds the specified shortcuts to the CLI, and updates to the cli.ts file that use this function to bind the default set of shortcuts as well as an additional shortcut to stop the profiler, if it is running.

One issue with this patch is that it uses the global object to store the Session object used by the profiler. This is not recommended, as it can lead to variable name collisions and other issues if multiple modules use the global object in this way. It would be better to pass the Session object as an argument to the stopProfiler function, rather than using the global object.

Another issue is that the stopProfiler function takes a log function as an argument, but the implementation of the shortcut that calls this function does not pass a function. Instead, it passes the result of calling the info method on the server.config.logger object, which is not a function. This will cause an error at runtime. To fix this, the info method should be called within the stopProfiler function, rather than passing the method itself.

Aside from these issues, the changes in this patch appear to be well-structured and well-written. Adding keyboard shortcut support is a useful feature, and the implementation looks correct.

aleclarson
aleclarson previously approved these changes Dec 7, 2022
@benmccann
Copy link
Collaborator

Looks like ChatGPT was 0/2 on its code review suggestions, which are both wrong

Comment on lines +42 to +47
// ctrl+c or ctrl+d
if (input === '\x03' || input === '\x04') {
process.emit('SIGTERM')
return
}
Copy link
Member

Choose a reason for hiding this comment

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

The node docs showed that it's commonly emitted as SIGINT for all platforms. Seems like SIGTERM isn't supported in Windows too. Should we change this to SIGINT?

With that said we only listen for SIGTERM for the server today (not sure if that has been an issue in Windows), but I think it's nice to follow the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will test if it's still exit cleanly on mac os with SIGINT

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't close at all when emitting SIGINT.

Doing await server.close().finally(() => process.exit()) works fine, but maybe not perfect for middleware mode

packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
@@ -60,6 +60,7 @@ export type {
LogType,
LoggerOptions,
} from './logger'
export type { BindShortcutsOptions, CLIShortcut } from './shortcuts'
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed with the internal change too.

Copy link
Member Author

Choose a reason for hiding this comment

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

The build process force me to expose it even when it's internal. Do you know how to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

Ah this issue. Maybe we could refactor a bit to have bindShortcuts() do a server._shortcutsBound = true in the function itself? That way we can remove

bindShortcuts(opts = {}) {
bindShortcuts(server, opts)
server._shortcutsBound = true
},
and the types so it doesn't get bundled by api extractor.

Otherwise the other option is to post process and strip it off like

// remove picomatch type import because only the internal property uses it
const picomatchImport = "import type { Matcher as Matcher_2 } from 'picomatch';"

The first one would be easier and also prevents someone accessing it.

Copy link
Member

Choose a reason for hiding this comment

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

You can also hide it in a weak map against the server, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Found something else, tell me what you think

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Dec 7, 2022

I updated the custom profiler shortcut to enable profiler restart/stop and allow to create profile session for HMR updates

Screenshot 2022-12-07 at 19 53 57

@patak-dev
Copy link
Member

Works great! I think we should merge and release it so we can give at least a day to the ecosystem to test it.

@ArnaudBarre if you would like, maybe we could add in another PR a title when pressing h. So it looks like in Vitest
image

Right now it looks a bit confusing when pressing twice (although it is an edge case)
image

but the extra line and title help while reading the options. We could also align the options so h and p, etc are all on the same col.

@patak-dev patak-dev merged commit 87973f1 into main Dec 7, 2022
@patak-dev patak-dev deleted the cli-shortcuts branch December 7, 2022 19:53
@ArnaudBarre
Copy link
Member Author

Yeah we need to know if ctrl+c works on Windows.

I think that with blank line + title we don't need to align. Will to it in a next PR

@dbousamra
Copy link

Hi all. I believe this PR has broken Ctrl-C behaviour somehow. I notice if I run 2 vites servers using tools like yarn workspace foreach or even just npm-run-all where I run vite with another command, vite will not shutdown with ctrl-c.

@patak-dev
Copy link
Member

@dbousamra I see you also found #11563, a fix on top of this PR. If you have a diff repro from #11434 or you are seeing a regression from it, please create an issue with a new repro against the latest main so we can track it. A comment in an old PR would get lost quickly.

@dbousamra
Copy link

@patak-dev False alarm. Was actually an issue with vitest, not yarn. Was very confused

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.

None yet

7 participants