[explorer] add local explorer hotkey to the vite plugin#13137
[explorer] add local explorer hotkey to the vite plugin#13137emily-shen wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 0e4fb82 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
✅ All changesets look good |
emily-shen
force-pushed
the
emily/vite-hotkey
branch
from
bc8e89a to
c2e5e43
Compare
|
I've now thoroughly reviewed all the changes. Let me summarize my findings: The implementation is clean and follows existing patterns well. The code:
The only thing I notice is not really an issue but worth noting: in wrangler, the explorer hotkey has its label intentionally hidden (commented out) to keep it secret before announcement, while the vite plugin has a visible LGTM |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| server.bindCLIShortcuts({ | ||
| customShortcuts: [openExplorerShortcut], | ||
| }); |
There was a problem hiding this comment.
🔴 addExplorerShortcut chains through addBindingsShortcut wrapper, calling original bindCLIShortcuts with only the explorer shortcut
When both addBindingsShortcut and addExplorerShortcut are called sequentially (as in configureServer at packages/vite-plugin-cloudflare/src/plugins/shortcuts.ts:26-27 and configurePreviewServer at lines 35-36), the explorer function's call to server.bindCLIShortcuts({ customShortcuts: [openExplorerShortcut] }) at line 170-172 chains through the bindings wrapper (installed at line 88-110) which forwards the call to Vite's original bindCLIShortcuts with only [openExplorerShortcut]. If Vite's bindCLIShortcuts replaces (rather than accumulates) custom shortcuts on each invocation, the previously registered bindings shortcut (b) is lost. The fix is to either register both shortcuts in a single call, or have each wrapper merge its own shortcut into the customShortcuts array before forwarding.
Trace of the call chain
addBindingsShortcutwraps original and calls:origBind({ customShortcuts: [b] })addExplorerShortcutwraps the bindings wrapper and calls:explorerWrapper({ customShortcuts: [e] })→bindingsWrapper({ customShortcuts: [e] })→origBind({ customShortcuts: [e] })
The [b] shortcut from step 1 is not included in step 2's call to the original.
Prompt for agents
In packages/vite-plugin-cloudflare/src/plugins/shortcuts.ts, the addExplorerShortcut function (lines 118-173) and addBindingsShortcut function (lines 41-116) each independently wrap server.bindCLIShortcuts and call it with their own shortcut. When addExplorerShortcut chains through addBindingsShortcut's wrapper, the original bindCLIShortcuts is called with only the explorer shortcut, potentially losing the bindings shortcut.
The recommended fix is to have the wrapper functions merge their own shortcuts into any customShortcuts that are passed through. Specifically, in each wrapper (lines 88-110 and lines 148-168), before calling the saved bindCLIShortcuts, merge the plugin's own shortcut into options.customShortcuts. For example, change the bindings wrapper to:
bindCLIShortcuts({
...options,
customShortcuts: [
...(options?.customShortcuts ?? []),
printBindingsShortcut,
],
});
And similarly for the explorer wrapper. Then change the final call in each function from server.bindCLIShortcuts({ customShortcuts: [shortcut] }) to just server.bindCLIShortcuts() (or server.bindCLIShortcuts({})), so the wrapper itself injects the shortcut.
Alternatively, combine both shortcuts into a single registration function that calls server.bindCLIShortcuts once with both customShortcuts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const childProcess = await open(explorerUrl); | ||
| childProcess.on("error", () => { | ||
| viteServer.config.logger.warn( | ||
| "Failed to open browser, the local explorer can be accessed at " + | ||
| explorerUrl | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Just as a sanity check: There's no additional process cleanup needed for this, correct? It's just cleaned up / killed once it's out of scope.
3 participants
this uses
elike in wrangler, which doesn't conflict with any default vite hotkeys.vite doesn't have a way of hiding bindings, so this should not be merged until we are ready to properly announce this.
Also, we seem to use
openin c3 and wrangler to open browsers, so i've used it here too in the vite plugin, and added it to the pnpm catalog and updated the version from 8 -> 11. open v11 is ESM only, but it seems fine? I vaguely remember we have tried to move off as many CJS packages as possible, not sure if this was skipped on purpose.A picture of a cute animal (not mandatory, but encouraged)