Conversation
|
|
hookable to v6hookable to v6
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
Merging this PR will not alter performance
Comparing Footnotes
|
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an experimental 🚥 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)
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 |
95bde83 to
2af438f
Compare
40afc40 to
5e5e384
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt/src/app/nuxt.ts (1)
381-389:⚠️ Potential issue | 🟠 MajorServer-side hooks always return Promises, preventing compat-v5 sync-hook mode.
Because
contextCallerisasync, the server branch returns aPromisewhenever hooks are registered, even whenexperimental.asyncCallHookisfalse. This creates an asymmetry with the client branch, which respects the flag and preserves synchronous execution when false. Consequently, the compat-v5 synchronous hook path cannot be exercised on the server without architectural changes to how context is preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/app/nuxt.ts` around lines 381 - 389, The server branch uses an async contextCaller which forces Promise returns and breaks compat-v5 sync-hook mode; replace it with two variants and choose based on experimental.asyncCallHook: keep the existing async function (contextCallerAsync) that awaits each hook for async mode, and add a synchronous contextCallerSync that calls each hook inside nuxtApp.runWithContext without using await (e.g., for (const hook of hooks) hook(...args)), then set nuxtApp.hooks.callHook to use callHookWith(contextCallerSync, ...) when nuxtApp.options.experimental.asyncCallHook is false and to use the async variant otherwise, referencing contextCaller (or named variants), nuxtApp.hooks.callHook, and nuxtApp.hooks.callHookWith so the server preserves synchronous execution when requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nuxt/src/app/nuxt.ts`:
- Around line 390-392: The current replacement of nuxtApp.hooks.callHook wraps
the call synchronously using Promise.resolve(_callHook(name, ...args)) which
still allows synchronous throws to escape; update the override so it defers
execution into the microtask queue (e.g., use Promise.resolve().then(() =>
_callHook(name, ...args))) thereby ensuring any synchronous exceptions from
_callHook(name, ...args) are converted into rejected promises; modify the
assignment that references _callHook and asyncCallHook accordingly.
In `@packages/nuxt/src/core/nuxt.ts`:
- Around line 62-68: When options.experimental.asyncCallHook is true the current
use of Promise.resolve(runWithNuxtContext(...)) can create a Promise with a void
union and also lets synchronous errors escape, so change both assignments to
create the Promise first and defer invocation using Promise.resolve().then(() =>
runWithNuxtContext(...callHook...)). For callHookParallel ensure the resolved
value is narrowed to Promise<any[]> by using the nullish-coalescing fallback
(callHookParallel(...) ?? []) inside the .then to handle no-hook cases. Update
hooks.callHook and hooks.callHookParallel to use Promise.resolve().then(...)
with runWithNuxtContext and apply ?? [] for callHookParallel.
---
Outside diff comments:
In `@packages/nuxt/src/app/nuxt.ts`:
- Around line 381-389: The server branch uses an async contextCaller which
forces Promise returns and breaks compat-v5 sync-hook mode; replace it with two
variants and choose based on experimental.asyncCallHook: keep the existing async
function (contextCallerAsync) that awaits each hook for async mode, and add a
synchronous contextCallerSync that calls each hook inside nuxtApp.runWithContext
without using await (e.g., for (const hook of hooks) hook(...args)), then set
nuxtApp.hooks.callHook to use callHookWith(contextCallerSync, ...) when
nuxtApp.options.experimental.asyncCallHook is false and to use the async variant
otherwise, referencing contextCaller (or named variants),
nuxtApp.hooks.callHook, and nuxtApp.hooks.callHookWith so the server preserves
synchronous execution when requested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5321778-e0a8-4871-909c-e207e2233573
⛔ Files ignored due to path filters (4)
packages/kit/package.jsonis excluded by!**/package.jsonpackages/nuxt/package.jsonis excluded by!**/package.jsonpackages/schema/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (8)
docs/1.getting-started/18.upgrade.mdpackages/nuxt/src/app/components/nuxt-link.tspackages/nuxt/src/app/components/nuxt-root.vuepackages/nuxt/src/app/nuxt.tspackages/nuxt/src/core/nuxt.tspackages/nuxt/src/core/templates.tspackages/schema/src/config/experimental.tspackages/schema/src/types/schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/src/app/components/nuxt-root.vue
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nuxt/src/app/nuxt.ts`:
- Line 389: The hook wrapper passes the entire args array as a single argument
to callHookWith, causing nested arrays; update nuxtApp.hooks.callHook so it
forwards arguments using the spread operator to callHookWith (i.e.,
callHookWith(contextCaller, name, ...args)) so that callHookWith and
contextCaller receive individual variadic args rather than a single array;
modify the line assigning nuxtApp.hooks.callHook to use ...args and keep the
existing contextCaller and name parameters unchanged.
In `@packages/nuxt/src/core/nuxt.ts`:
- Line 80: The hook registration currently invokes nuxt.close() without awaiting
its Promise; update the hook callback in builder code that calls
nuxt.hook('prepare:types', ...) so it either returns the Promise or is declared
async and awaits nuxt.close() to ensure the 'close' hook completes; reference
the close method on the nuxt object and the nuxt.hook('prepare:types', ...)
registration when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ae560fc-811b-4602-b1c8-6310558aef84
📒 Files selected for processing (3)
packages/nuxt/src/app/nuxt.tspackages/nuxt/src/core/nuxt.tstest/mocks/nuxt-config.ts
| hook: hooks.hook, | ||
| ready: () => runWithNuxtContext(nuxt, () => initNuxt(nuxt)), | ||
| close: () => hooks.callHook('close', nuxt), | ||
| close: async () => { await hooks.callHook('close', nuxt) }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the nuxt.close() method definition
echo "=== Finding nuxt.close() method definition ==="
rg -n "close:\s*async" packages/nuxt/src/core/nuxt.ts -A 2 -B 2
echo -e "\n=== Checking the full method context around line 80 ==="
sed -n '75,85p' packages/nuxt/src/core/nuxt.tsRepository: nuxt/nuxt
Length of output: 671
🏁 Script executed:
#!/bin/bash
# Find all nuxt.close() calls in the codebase
echo "=== All nuxt.close() calls ==="
rg -n "nuxt\.close\(\)" packages/nuxt/src --type ts -B 2 -A 2Repository: nuxt/nuxt
Length of output: 403
🏁 Script executed:
#!/bin/bash
# Check the specific example in builder.ts line 62
echo "=== Checking builder.ts around line 62 ==="
sed -n '58,68p' packages/nuxt/src/core/builder.tsRepository: nuxt/nuxt
Length of output: 373
🏁 Script executed:
#!/bin/bash
# Search for all close() method calls (not just nuxt.close())
echo "=== All .close() calls in packages/nuxt/src ==="
rg -n "\.close\(\)" packages/nuxt/src --type ts -B 1 -A 1
echo -e "\n=== Check if there are other places calling nuxt.close ==="
rg -n "\.close\(" packages/nuxt/src --type ts | head -20Repository: nuxt/nuxt
Length of output: 2534
🏁 Script executed:
#!/bin/bash
# Check the broader context of the builder.ts issue - where does prepare:types get called
echo "=== Check where prepare:types hook is called ==="
rg -n "prepare:types" packages/nuxt/src --type ts -B 2 -A 2Repository: nuxt/nuxt
Length of output: 3248
Ensure nuxt.close() call site properly awaits the async method.
The close method is async and awaits the 'close' hook. The call at packages/nuxt/src/core/builder.ts (line 62) registers a hook callback without awaiting the result:
builder.ts line 62
nuxt.hook('prepare:types', () => nuxt.close())Since nuxt.close() returns a Promise that is not awaited, the cleanup scheduled by the hook will not complete before the callback returns, potentially leaving cleanup incomplete on process exit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nuxt/src/core/nuxt.ts` at line 80, The hook registration currently
invokes nuxt.close() without awaiting its Promise; update the hook callback in
builder code that calls nuxt.hook('prepare:types', ...) so it either returns the
Promise or is declared async and awaits nuxt.close() to ensure the 'close' hook
completes; reference the close method on the nuxt object and the
nuxt.hook('prepare:types', ...) registration when applying the change.
There was a problem hiding this comment.
that's not the case - nuxt.hook('prepare:types', () => nuxt.close()) means that when prepare:types is called, it awaits the close hook
There was a problem hiding this comment.
@danielroe, you're right — I apologize for the incorrect analysis. Since hookable's callHook awaits any Promise returned by a hook callback, returning nuxt.close() from the callback is sufficient to ensure the 'close' hook completes before the prepare:types hook finishes. No explicit async/await inside the callback is needed.
✏️ Learnings added
Learnt from: danielroe
Repo: nuxt/nuxt PR: 33905
File: packages/nuxt/src/core/nuxt.ts:80-80
Timestamp: 2026-03-06T14:21:08.891Z
Learning: In `nuxt/nuxt`, when registering a hook callback via `nuxt.hook(...)`, hookable's `callHook` automatically awaits any Promise returned by the callback. Therefore, patterns like `nuxt.hook('prepare:types', () => nuxt.close())` are correct and sufficient — the returned Promise from `nuxt.close()` will be awaited by hookable before proceeding, ensuring cleanup completes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: danielroe
Repo: nuxt/nuxt PR: 33905
File: packages/nuxt/src/app/nuxt.ts:389-389
Timestamp: 2026-03-06T14:19:09.012Z
Learning: In hookable v6 (unjs/hookable), `callHookWith` changed its signature from variadic spread args to a single pre-formed array parameter: `callHookWith(caller, name, args)` instead of v5's `callHookWith(caller, name, ...args)`. The caller function also gains a third `name` argument in v6: `(hooks, args, name) => any`. In `packages/nuxt/src/app/nuxt.ts`, passing `args` directly (not spread) to `callHookWith` is intentional and correct for hookable v6 compatibility.
🔗 Linked issue
📚 Description
splitting this out from #33005 to address any regressions separately