-
Notifications
You must be signed in to change notification settings - Fork 688
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
React-free hotkeys implementation #6149
Conversation
🦋 Changeset detectedLatest commit: 1887c45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-wrangler-6149 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6149/npm-package-wrangler-6149 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-wrangler-6149 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-create-cloudflare-6149 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-cloudflare-kv-asset-handler-6149 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-miniflare-6149 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-cloudflare-pages-shared-6149 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9763657552/npm-package-cloudflare-vitest-pool-workers-6149 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
189870c
to
50f1299
Compare
154ce0b
to
1079c6d
Compare
ded843c
to
8bb83d3
Compare
558c13a
to
8dccb57
Compare
|
||
if (process.stdin.isTTY) { | ||
readline.emitKeypressEvents(stream); | ||
process.stdin.setRawMode(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.
Should we capture what the current raw mode is to reuse in the teardown function?
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 reminds me I need to test this with the auth/account prompts
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 like we'll need to setRawMode(true) again after prompts have been used
(my manual testing just now found that prompts work fine but the hotkeys don't work after)
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.
3bc1fd1
to
ebafd22
Compare
packages/wrangler/src/cli-hotkeys.ts
Outdated
@@ -8,6 +8,7 @@ import type { Hook } from "./api"; | |||
export default function ( | |||
options: Array<{ | |||
keys: string[]; | |||
disabled?: 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.
Why do we have a disabled
option? Can't we just not pass it 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.
Felt it was easier to read a disabled flag in an flat-ordered list than the ternary+spread alternative
automatically compute instructions
for mocking
+ ensures no existing logs (from before registerGlobalBottomFloat was called) are cleared
+ use static methods of miniflare Log which are now stateful -- rather than reimplement in Wrangler's Logger not sharing state
6db7f95
to
4b4d56a
Compare
4b4d56a
to
1887c45
Compare
What this PR solves / how to test
Implements a React-free hotkeys implementation.
You can see both the implementations below – the React-free version with the
--x-dev-env
flag and the React/ink version with the flag:I started with a simpler implementation in the first commit but felt a higher-level API would allow for most consistency for other commands wanting to use hotkeys. The instructions printing logic has been moved inside the API and provides each handler with a
printInstructions
function if the handler wishes to reprint them.UPDATE:
The instructions box now floats at the bottom (by making Wrangler's
Logger
and Miniflare'sLog
classes aware of a possible bottom float string). See image below for what it looks like now (note the worker logs are now above the instructions box):Author has addressed the following