Merged
Conversation
Replace the heavier execa dependency with the lightweight tinyexec alternative. tinyexec is already available as a transitive dependency via @antfu/ni. Breaking changes: - SubprocessOptions no longer extends ExecaOptions; now has its own env and nodeOptions fields - startSubprocess().getProcess() is deprecated (use getResult() for full Result API) - Subprocess stream access is now via result.process?.stdout instead of result.stdout Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
antfu
changed the title
Deploying nuxt-devtools with
|
| Latest commit: |
b3b7595
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4d0e6784.nuxt-devtools.pages.dev |
| Branch Preview URL: | https://antfu-migrate-execa-to-tinye.nuxt-devtools.pages.dev |
- Add `cwd` field to SubprocessOptions for backward compat - Migrate getProcess() callers to getResult() in analyze-build and npm RPCs - Move `stdio` option to `nodeOptions` in local.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request migrates the project from the Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/2.module/3.migration-v3.md`:
- Around line 1-4: The frontmatter title "Migration from v3" is misleading;
update the title field in the document's frontmatter (the top YAML block
`title:`) to a clearer option such as "Migration to v3", "v3 Migration Guide",
or "Upgrading to v3" so the heading correctly reflects that the doc describes
changes introduced in v3.
- Around line 38-52: Update the migration note for startSubprocess() to document
that getResult() returns a tinyexec Result which is promise-like and can be
awaited to obtain the exit code; reference startSubprocess(), getProcess(),
getResult(), and the Result object and explain in one sentence that callers
should either await subprocess.getResult() (to read result.exitCode) or use the
promise-like then(...) form, and show that stream access remains available via
result.process?.stdout.
In `@packages/devtools-kit/src/_types/terminals.ts`:
- Around line 12-17: SubprocessOptions no longer accepts top-level cwd — it must
be placed inside nodeOptions (which is Node.js SpawnOptions); find the three
places that construct a SubprocessOptions object (the callsites still passing
cwd at the top level) and move the cwd value into nodeOptions: { cwd: ... }
(ensure nodeOptions exists or create it), preserving any other SpawnOptions
there; update the objects used in analyze-build call, the local subprocess
invocation, and the playground nuxt starter invocation so they pass cwd only via
nodeOptions and satisfy the SpawnOptions type.
In `@packages/devtools-kit/src/index.ts`:
- Line 74: Fix the typo in the terminal exit message by changing the string
passed to nuxt.callHook in the code that emits the terminal write (the call
using nuxt.callHook('devtools:terminal:write', { id, data: `\n> process
terminalated with ${code}\n` })). Update "terminalated" to "terminated" so the
message reads "\n> process terminated with ${code}\n".
- Around line 51-60: The current spread order allows execaOptions.nodeOptions to
overwrite the constructed nodeOptions.env entirely; change the construction of
nodeOptions so any env provided on execaOptions.nodeOptions.env is merged into
the built env instead of replacing it. Concretely, build nodeOptions by first
spreading execaOptions.nodeOptions (for other nodeOptions fields) but set env
explicitly to a merged object like { ...process.env, COLORS: 'true',
FORCE_COLOR: 'true', ...execaOptions.env, __CLI_ARGV__: undefined,
...execaOptions.nodeOptions?.env } so that the default/forced variables are
preserved and any user-provided env entries are merged in; keep references to
nodeOptions, execaOptions, env, and __CLI_ARGV__ to locate the change.
- Around line 36-38: getProcess() now returns a ChildProcess | undefined (not a
Promise/Result), so update all callers to use getResult() which returns the
awaitable Result; specifically, replace patterns that do const execa =
process.getProcess(); await execa or process.getProcess().then(...) with const
result = await subprocess.getResult(); also add guards for an undefined process
if you rely on getProcess() directly (handle undefined or obtain the
ChildProcess via other API), and update any type annotations/usages in
analyze-build (calls using .then) and npm-related callers (places that awaited
getProcess()) to use getResult() instead.
In `@packages/devtools/src/integrations/vscode.ts`:
- Around line 89-93: The call to x(codeBinary, ['--install-extension',
'antfu.vscode-server-controller'], ...) is fire-and-forget and can produce
unhandled rejections; wrap this invocation in an explicit await with a try/catch
(or attach .catch) to handle subprocess spawn/exit failures, log the error via
the existing logger, and surface a clear failure path (e.g., fallback or
process.exit) so startup doesn’t proceed silently on failure; locate the call to
x and update it to catch and handle both spawn errors and non-zero exits for
codeBinary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74469b5d-d806-4cab-b570-a01f95628991
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
docs/content/2.module/1.utils-kit.mddocs/content/2.module/3.migration-v3.mdpackage.jsonpackages/devtools-kit/package.jsonpackages/devtools-kit/src/_types/terminals.tspackages/devtools-kit/src/index.tspackages/devtools-wizard/package.jsonpackages/devtools/package.jsonpackages/devtools/src/integrations/vscode.tspnpm-workspace.yaml
Comment on lines
+38
to
+52
| ### `getProcess()` is deprecated | ||
|
|
||
| The return value of `startSubprocess()` now provides `getResult()` instead of `getProcess()`. | ||
|
|
||
| - `getProcess()` still works but logs a deprecation warning and returns `ChildProcess | undefined` (was `ExecaChildProcess<string>`) | ||
| - `getResult()` returns a tinyexec `Result` object with `.kill()`, `.process`, `.pipe()`, and more | ||
|
|
||
| ```diff | ||
| const subprocess = startSubprocess(/* ... */) | ||
|
|
||
| - const proc = subprocess.getProcess() | ||
| - proc.stdout.on('data', handler) | ||
| + const result = subprocess.getResult() | ||
| + result.process?.stdout?.on('data', handler) | ||
| ``` |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the awaitable pattern for exit code access.
The existing callers (see npm.ts, analyze-build.ts) await the result to access .exitCode. The migration guide shows stream access but misses this critical pattern. Since Result from tinyexec is Promise-like, this should be documented.
Suggested addition
- `getResult()` returns a tinyexec `Result` object with `.kill()`, `.process`, `.pipe()`, and more
+- `Result` is Promise-like and can be awaited to get the final exit code
```diff
const subprocess = startSubprocess(/* ... */)
- const proc = subprocess.getProcess()
- proc.stdout.on('data', handler)
+ const result = subprocess.getResult()
+ result.process?.stdout?.on('data', handler)
```
+
+If you were awaiting the process to get the exit code:
+
+```diff
+const subprocess = startSubprocess(/* ... */)
+
+- const proc = subprocess.getProcess()
+- const result = await proc
+- console.log(result.exitCode)
++ const result = await subprocess.getResult()
++ console.log(result.exitCode)
+```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/2.module/3.migration-v3.md` around lines 38 - 52, Update the
migration note for startSubprocess() to document that getResult() returns a
tinyexec Result which is promise-like and can be awaited to obtain the exit
code; reference startSubprocess(), getProcess(), getResult(), and the Result
object and explain in one sentence that callers should either await
subprocess.getResult() (to read result.exitCode) or use the promise-like
then(...) form, and show that stream access remains available via
result.process?.stdout.
Comment on lines
+89
to
93
| x(codeBinary, [ | ||
| '--install-extension', | ||
| 'antfu.vscode-server-controller', | ||
| ], { stderr: 'inherit', stdout: 'ignore', reject: false }) | ||
| ], { nodeOptions: { stdio: ['pipe', 'ignore', 'inherit'] } }) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In tinyexec, does x()reject on spawn errors by default, and isthrowOnErrordefaultfalse for non-zero exit codes?
💡 Result:
Yes.
- Spawn errors:
x()attaches an'error'listener to the spawnedChildProcess; if that fires, it stores the error and then throws it when youawaitthe result / iterate, so the returned promise rejects by default on spawn (and other process) errors. [1] - Non‑zero exit codes:
throwOnErroronly triggers whenoptions.throwOnErroris truthy; since the library’sdefaultOptionsdon’t setthrowOnError, the effective default isfalse(non-zero exit codes do not throw unless you opt in). [1][2]
Sources: [1] src/main.ts (tinylibs/tinyexec) implementation, [2] README option docs.
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
cat -n packages/devtools/src/integrations/vscode.ts | sed -n '85,100p'Repository: nuxt/devtools
Length of output: 644
🏁 Script executed:
# Check the parent function context and any try-catch wrapping
cat -n packages/devtools/src/integrations/vscode.ts | sed -n '70,110p'Repository: nuxt/devtools
Length of output: 1543
🏁 Script executed:
# Check imports and logger/LOG_PREFIX definition at the top of the file
head -50 packages/devtools/src/integrations/vscode.tsRepository: nuxt/devtools
Length of output: 2088
Handle the extension-install subprocess failure path explicitly.
At lines 89-92, x(...) is fire-and-forget with no error handling. Since x() rejects on spawn errors by default (e.g., binary not found, permission denied), an unhandled rejection can occur, causing process instability or flaky startup diagnostics.
💡 Suggested fix
// Install VS Code Server Controller
// https://github.com/antfu/vscode-server-controller
- x(codeBinary, [
+ void x(codeBinary, [
'--install-extension',
'antfu.vscode-server-controller',
- ], { nodeOptions: { stdio: ['pipe', 'ignore', 'inherit'] } })
+ ], { nodeOptions: { stdio: ['pipe', 'ignore', 'inherit'] } })
+ .catch((error) => {
+ logger.warn(LOG_PREFIX, `Failed to install VS Code Server Controller: ${String(error)}`)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| x(codeBinary, [ | |
| '--install-extension', | |
| 'antfu.vscode-server-controller', | |
| ], { stderr: 'inherit', stdout: 'ignore', reject: false }) | |
| ], { nodeOptions: { stdio: ['pipe', 'ignore', 'inherit'] } }) | |
| void x(codeBinary, [ | |
| '--install-extension', | |
| 'antfu.vscode-server-controller', | |
| ], { nodeOptions: { stdio: ['pipe', 'ignore', 'inherit'] } }) | |
| .catch((error) => { | |
| logger.warn(LOG_PREFIX, `Failed to install VS Code Server Controller: ${String(error)}`) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools/src/integrations/vscode.ts` around lines 89 - 93, The call
to x(codeBinary, ['--install-extension', 'antfu.vscode-server-controller'], ...)
is fire-and-forget and can produce unhandled rejections; wrap this invocation in
an explicit await with a try/catch (or attach .catch) to handle subprocess
spawn/exit failures, log the error via the existing logger, and surface a clear
failure path (e.g., fallback or process.exit) so startup doesn’t proceed
silently on failure; locate the call to x and update it to catch and handle both
spawn errors and non-zero exits for codeBinary.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/devtools-kit/src/_types/terminals.ts (1)
12-17: Interface design introduces ambiguity between top-level and nested options.Both
cwdandnodeOptions(which acceptsSpawnOptionsincludingcwd) are available. The implementation inindex.tsplaces the top-levelcwdintonodeOptions.cwd, then spreadsnodeOptionsafter—meaningnodeOptions.cwdwould take precedence if both are provided.Consider documenting the precedence or removing the redundancy. For now, this is acceptable given backward compatibility needs.
📝 Suggested documentation
export interface SubprocessOptions { command: string args?: string[] + /** Working directory. If `nodeOptions.cwd` is also set, `nodeOptions.cwd` takes precedence. */ cwd?: string + /** Environment variables merged with `process.env`. If `nodeOptions.env` is also set, it is merged. */ env?: Record<string, string | undefined> nodeOptions?: SpawnOptions }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools-kit/src/_types/terminals.ts` around lines 12 - 17, SubprocessOptions exposes both a top-level cwd and nodeOptions (which itself contains cwd), causing ambiguity; update the SubprocessOptions interface documentation to state that the implementation maps the top-level cwd into nodeOptions.cwd but any cwd provided inside nodeOptions takes precedence (i.e., nodeOptions.cwd wins when both are present), and add a brief comment above SubprocessOptions and/or in the implementation in index.ts explaining this precedence to avoid confusion while preserving backward compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/devtools-kit/src/_types/terminals.ts`:
- Around line 12-17: SubprocessOptions exposes both a top-level cwd and
nodeOptions (which itself contains cwd), causing ambiguity; update the
SubprocessOptions interface documentation to state that the implementation maps
the top-level cwd into nodeOptions.cwd but any cwd provided inside nodeOptions
takes precedence (i.e., nodeOptions.cwd wins when both are present), and add a
brief comment above SubprocessOptions and/or in the implementation in index.ts
explaining this precedence to avoid confusion while preserving backward
compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 271ef888-73f7-43e1-b537-bbb5123226eb
📒 Files selected for processing (5)
local.tspackages/devtools-kit/src/_types/terminals.tspackages/devtools-kit/src/index.tspackages/devtools/src/server-rpc/analyze-build.tspackages/devtools/src/server-rpc/npm.ts
… v4" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the heavier
execadependency with the lightweighttinyexecalternative.tinyexecis already available as a transitive dependency via@antfu/ni, reducing overall bundle size.Breaking Changes
SubprocessOptionsno longer extendsExecaOptions; now has its ownenvandnodeOptionsfieldsstartSubprocess().getProcess()is deprecated (usegetResult()for full tinyexecResultAPI)result.process?.stdoutinstead ofresult.stdoutCompatibility Layer
The
getProcess()method remains functional but logs a deprecation warning, easing the migration path for consumers.🤖 Generated with Claude Code