fix(nuxt): print route middleware path in warning #33136
Conversation
|
|
WalkthroughThe middleware template in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nuxt/src/core/templates.ts (1)
341-341: Prefer knitwork helpers over JSON.stringify for consistencyMinor style nit: use
genArrayFromRaw+genStringto mirror surrounding codegen patterns and ensure consistent escaping.Apply:
- `export const globalMiddlewareNameMap = ${JSON.stringify(globalMiddleware.map(mw => mw.name))}`, + `export const globalMiddlewareNameMap = ${genArrayFromRaw(globalMiddleware.map(mw => genString(mw.name)))}`,packages/nuxt/src/pages/runtime/plugins/router.ts (1)
225-229: Avoid unnecessary indexOf for string entriesTiny optimisation/readability: only look up the index for function entries; strings cannot be in
globalMiddleware.Apply:
- if (import.meta.dev) { - nuxtApp._processingMiddleware = globalMiddlewareNameMap[globalMiddleware.indexOf(entry)] || (typeof entry === 'string' ? entry : middleware.name) || true - } + if (import.meta.dev) { + const idx = typeof entry !== 'string' ? globalMiddleware.indexOf(entry) : -1 + nuxtApp._processingMiddleware = + (idx !== -1 ? globalMiddlewareNameMap[idx] : undefined) || + (typeof entry === 'string' ? entry : middleware.name) || + true + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/nuxt/src/app/plugins/router.ts(2 hunks)packages/nuxt/src/core/templates.ts(1 hunks)packages/nuxt/src/pages/runtime/plugins/router.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/core/templates.tspackages/nuxt/src/pages/runtime/plugins/router.tspackages/nuxt/src/app/plugins/router.ts
🔇 Additional comments (4)
packages/nuxt/src/core/templates.ts (1)
341-341: LGTM: exposing a stable name map for dev tooling is sensibleThe export aligns well with how
globalMiddlewareis generated and should improve _processingMiddleware labelling in dev.packages/nuxt/src/app/plugins/router.ts (2)
11-11: LGTM: importing the name map from the generated moduleThe new import is correctly gated by the existing virtual-file comment and matches the middleware template export.
271-271: Dev-only labelling now maps by index — good fallback behaviourUsing the index into
globalMiddlewarewith a fallback tomiddleware.name || trueis robust and keeps behaviour unchanged outside dev.packages/nuxt/src/pages/runtime/plugins/router.ts (1)
22-22: LGTM: added import for the name mapMatches the new export in
#build/middlewareand keeps the virtual-file typing suppression consistent.
CodSpeed Performance ReportMerging #33136 will not alter performanceComparing Summary
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/bundle.test.ts (2)
41-41: Avoid snapshot brittleness for bundle size; assert within a small range instead.Bundle size tends to drift by ~1–2 kB with harmless codegen changes; using a strict inline snapshot makes this test flaky.
- expect.soft(roundToKilobytes(clientStats!.totalBytes)).toMatchInlineSnapshot(`"171k"`) + // Allow minor variance from code generation and dependency updates + expect.soft(clientStats!.totalBytes).toBeGreaterThanOrEqual(170 * 1024) + expect.soft(clientStats!.totalBytes).toBeLessThanOrEqual(173 * 1024)
98-98: Use numeric bounds instead of an exact snapshot for server bundle size (inlined).Small, non-functional changes can shift this value; prefer a tolerance to reduce flaky failures.
- expect.soft(roundToKilobytes(serverStats.totalBytes)).toMatchInlineSnapshot(`"549k"`) + // Allow minor variance from bundler/codegen changes + expect.soft(serverStats.totalBytes).toBeGreaterThanOrEqual(546 * 1024) + expect.soft(serverStats.totalBytes).toBeLessThanOrEqual(552 * 1024)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/bundle.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/bundle.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/bundle.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
BobbieGoede
changed the title
packages/nuxt/src/core/templates.ts
Outdated
| `for (const name in _globalMiddleware) {`, | ||
| ` Object.defineProperty(_globalMiddleware[name], 'name', { value: name })`, | ||
| `}`, |
There was a problem hiding this comment.
we should only do this in dev mode, and likely only if the function doesn't already have a name....
There was a problem hiding this comment.
done! now it uses the original export/implementation outside dev mode, this way the dev logic/variable is tree-shaken out.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nuxt/src/core/templates.ts (1)
342-347: Dev-only and “only-if-unnamed” logic acknowledged.This aligns with prior feedback; keep this behaviour with the array-based approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/nuxt/src/core/templates.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/core/templates.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code
|
have I made it too complicated? it prints out this: 
previously: 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/nuxt/src/pages/runtime/plugins/router.ts (1)
225-229: Preserve fallback to middleware function name for better dev diagnosticsIf
_pathis missing (e.g. programmatically-registered middleware), falling back totruedegrades warning clarity. Keep the previousmiddleware.namefallback.- nuxtApp._processingMiddleware = (middleware as any)._path || (typeof entry === 'string' ? entry : true) + nuxtApp._processingMiddleware = (middleware as any)._path + || (typeof entry === 'string' ? entry : (middleware as any).name) + || truepackages/nuxt/src/app/plugins/router.ts (1)
269-273: Retain name fallback when_pathis absentSame rationale as pages runtime: keep
middleware.namebefore falling back totrueto improve dev-time warnings.- nuxtApp._processingMiddleware = (middleware as any)._path || true + nuxtApp._processingMiddleware = (middleware as any)._path + || (middleware as any).name + || truepackages/nuxt/src/core/templates.ts (2)
346-364: Make_pathproperties configurable to avoid dev-time surprisesDefining
_pathas non-configurable/non-writable can hinder HMR or wrappers that may want to redefine it. Mark asconfigurable: true.- for (const path in _globalMiddleware) { - Object.defineProperty(_globalMiddleware[path], '_path', { value: path }) - } + for (const path in _globalMiddleware) { + Object.defineProperty(_globalMiddleware[path], '_path', { value: path, configurable: true }) + } @@ - mw.import = () => i().then(r => { - Object.defineProperty(r.default || r, '_path', { value: mw.path }) - return r - }) + mw.import = () => i().then(r => { + Object.defineProperty(r.default || r, '_path', { value: mw.path, configurable: true }) + return r + })
677-680: Tiny doc hint for maintainersAdd a short comment explaining why
@/@@are stripped (to surface real file-ish paths in warnings).-const strippedAtAliases = { +// Normalise common Nuxt aliases in dev warnings for readability +const strippedAtAliases = { '@': '', '@@': '', }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/nuxt/src/app/plugins/router.ts(1 hunks)packages/nuxt/src/core/templates.ts(3 hunks)packages/nuxt/src/pages/runtime/plugins/router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/app/plugins/router.tspackages/nuxt/src/pages/runtime/plugins/router.tspackages/nuxt/src/core/templates.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (4)
packages/nuxt/src/core/templates.ts (4)
9-9: LGTM: importreverseResolveAliasImport location is correct and used only in dev code paths.
334-334: LGTM: widenedgetContentssignatureUsing
{ app, nuxt }is compatible with existing template invocation and required for alias handling.
337-337: LGTM: dev-only alias mapMerging
nuxt.options.aliaswith stripped@/@@is sensible for user-facing paths in warnings.
346-351: Order and deduplication checkUsing an object keyed by resolved path and
Object.valuespreserves insertion order but will drop duplicates sharing the same resolved path. That’s unlikely in practice; calling it out for awareness.Would you like me to add a tiny generator-side assertion to warn if duplicate paths are detected in dev?
danielroe
changed the title
|
We could print/add the full path of the middleware to the stack trace if it isn't in there too 👀 the alias path is nice but it doesn't click through to file (at least in my local project) This is what is printed with the reproduction from #31029 |
|
tricky one the difficulty is - sometimes middleware will already be in the stack trace, but if it isn't, then it's not part of the call stack at all (e.g. separated with an async call somewhere) so it would be misleading to print it. |
|
Let's stick with the alias path in the warning itself, as this is what we do for other warnings. |
2 participants
🔗 Linked issue
useRouteusage warning #33039📚 Description
This may work without too much changes, but it feels suboptimal.
With these changes, the warning trace will include the name of the global middleware.