perf(nuxt,schema): normalise components + directories more efficiently #33207
Conversation
|
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33207 will not alter performanceComparing Summary
Footnotes |
|
I'll move the change to when the hook is called into a separate PR so we can discuss properly. |
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/nuxt/src/components/module.ts (4)
28-28: Windows path handling in regex.
DEFAULT_COMPONENTS_DIRS_REonly matches/; on Windows this can cause false warnings for default dirs.Apply:
-const DEFAULT_COMPONENTS_DIRS_RE = /\/components(?:\/(?:global|islands))?$/ +const DEFAULT_COMPONENTS_DIRS_RE = /[\\/]+components(?:[\\/](?:global|islands))?$/
83-85: Deduplicate transpile entries.Avoid pushing duplicate strings into
build.transpile.Apply:
- if (transpile) { - nuxt.options.build.transpile.push(dirPath) - } + if (transpile && !nuxt.options.build.transpile.includes(dirPath)) { + nuxt.options.build.transpile.push(dirPath) + }
21-21: Type guard can narrow toComponentsDir | string.This improves downstream inference when accessing
pathon object cases.Apply:
-const isPureObjectOrString = (val: unknown): val is object | string => (!Array.isArray(val) && typeof val === 'object') || typeof val === 'string' +const isPureObjectOrString = (val: unknown): val is ComponentsDir | string => + typeof val === 'string' || (!!val && !Array.isArray(val) && typeof val === 'object')
263-297: Normalisation looks sound; minor note on double alias resolution.
normalizeDirsresolves aliases and makes absolute paths, and later the loop resolves aliases again. It’s harmless, but you could skip the second resolution whendirOptions.pathis already absolute for a tiny win.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/components/module.ts(5 hunks)packages/schema/src/types/config.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/schema/src/types/config.tspackages/nuxt/src/components/module.ts
🧠 Learnings (1)
📚 Learning: 2024-11-05T15:22:54.759Z
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.
Applied to files:
packages/nuxt/src/components/module.ts
🧬 Code graph analysis (1)
packages/nuxt/src/components/module.ts (5)
packages/kit/src/index.ts (3)
addBuildPlugin(20-20)resolveAlias(32-32)logger(36-36)packages/kit/src/build.ts (1)
addBuildPlugin(153-165)packages/nuxt/src/components/plugins/component-names.ts (1)
ComponentNamePlugin(16-68)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/vite/src/utils/transpile.ts (1)
transpile(11-28)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: codeql (actions)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: code
🔇 Additional comments (3)
packages/schema/src/types/config.ts (1)
49-50: ExplicitcomponentsonNuxtConfiglooks correct and improves type fidelity.Re-introducing
components?: ConfigSchema['components']while omitting it from theDeepPartialbase avoids accidental deep-partialing of the union and aligns with the schema. No further action from my side.packages/nuxt/src/components/module.ts (2)
111-114: Ordering user before library dirs is a solid default.This preserves intuitive resolution precedence. LGTM.
128-130: Switch tomoduleOptions.generateMetadatais consistent.Matches the module generic and top-level config changes. No issues spotted.
| const dirs = dir.path.includes('node_modules') ? libraryComponentDirs : userComponentDirs | ||
|
|
||
| dirs.push({ | ||
| global: moduleOptions.global, | ||
| ...dirOptions, | ||
| // TODO: https://github.com/nuxt/framework/pull/251 | ||
| enabled: true, | ||
| path: dirPath, | ||
| extensions, | ||
| pattern: dirOptions.pattern || `**/*.{${extensions.join(',')},}`, | ||
| pattern: dirOptions.pattern || (extensions.length > 1 ? `**/*.{${extensions.join(',')}}` : `**/*.${extensions[0] || '*'}`), | ||
| ignore: [ | ||
| '**/*{M,.m,-m}ixin.{js,ts,jsx,tsx}', // ignore mixins | ||
| '**/*.d.{cts,mts,ts}', // .d.ts files | ||
| ...(dirOptions.ignore || []), | ||
| ], | ||
| transpile: (transpile === 'auto' ? dirPath.includes('node_modules') : transpile), | ||
| } | ||
| }).filter(d => d.enabled) | ||
| transpile, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: using dir.path can crash when a hook adds string entries.
After components:dirs, allDirs may include strings. The guard wraps them into dirOptions, but Line 92 reads dir.path (possibly undefined). Use dirOptions.path.
Apply:
- const dirs = dir.path.includes('node_modules') ? libraryComponentDirs : userComponentDirs
+ const dirs = dirOptions.path.includes('node_modules') ? libraryComponentDirs : userComponentDirs📝 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.
| const dirs = dir.path.includes('node_modules') ? libraryComponentDirs : userComponentDirs | |
| dirs.push({ | |
| global: moduleOptions.global, | |
| ...dirOptions, | |
| // TODO: https://github.com/nuxt/framework/pull/251 | |
| enabled: true, | |
| path: dirPath, | |
| extensions, | |
| pattern: dirOptions.pattern || `**/*.{${extensions.join(',')},}`, | |
| pattern: dirOptions.pattern || (extensions.length > 1 ? `**/*.{${extensions.join(',')}}` : `**/*.${extensions[0] || '*'}`), | |
| ignore: [ | |
| '**/*{M,.m,-m}ixin.{js,ts,jsx,tsx}', // ignore mixins | |
| '**/*.d.{cts,mts,ts}', // .d.ts files | |
| ...(dirOptions.ignore || []), | |
| ], | |
| transpile: (transpile === 'auto' ? dirPath.includes('node_modules') : transpile), | |
| } | |
| }).filter(d => d.enabled) | |
| transpile, | |
| }) | |
| } | |
| const dirs = dirOptions.path.includes('node_modules') ? libraryComponentDirs : userComponentDirs | |
| dirs.push({ | |
| global: moduleOptions.global, | |
| ...dirOptions, | |
| // TODO: https://github.com/nuxt/framework/pull/251 | |
| enabled: true, | |
| path: dirPath, | |
| extensions, | |
| pattern: dirOptions.pattern || (extensions.length > 1 ? `**/*.{${extensions.join(',')}}` : `**/*.${extensions[0] || '*'}`), | |
| ignore: [ | |
| '**/*{M,.m,-m}ixin.{js,ts,jsx,tsx}', // ignore mixins | |
| '**/*.d.{cts,mts,ts}', // .d.ts files | |
| ...(dirOptions.ignore || []), | |
| ], | |
| transpile, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In packages/nuxt/src/components/module.ts around lines 92 to 109, the code reads
dir.path which can be undefined when a hook adds string entries (allDirs may
include strings); update the logic to use dirOptions.path (the normalized
object) instead of dir.path when deciding between libraryComponentDirs and
userComponentDirs and when pushing the entry so you only access the validated
properties on dirOptions; ensure any other references in this block use
dirOptions rather than dir to avoid crashes from string entries.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @danielroe. * #33207 (comment) The following files were modified: * `packages/nuxt/src/components/module.ts`
|
Note Generated docstrings for this pull request at #33209 |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/nuxt/src/components/module.ts (4)
90-95: Harden node_modules detection and avoid duplicate transpile entries.
includes('node_modules')can false-match; check path segments.- Guard against pushing the same path multiple times to
build.transpile.-const _transpile = typeof dirOptions.transpile === 'boolean' ? dirOptions.transpile : 'auto' -const transpile = _transpile === 'auto' ? dirPath.includes('node_modules') : _transpile -if (transpile) { - nuxt.options.build.transpile.push(dirPath) -} +const _transpile = typeof dirOptions.transpile === 'boolean' ? dirOptions.transpile : 'auto' +const inNodeModules = dirPath.split(SLASH_SEPARATOR_RE).includes('node_modules') +const transpile = _transpile === 'auto' ? inNodeModules : _transpile +if (transpile && !nuxt.options.build.transpile.includes(dirPath)) { + nuxt.options.build.transpile.push(dirPath) +} @@ -const dirs = dirPath.includes('node_modules') ? libraryComponentDirs : userComponentDirs +const dirs = inNodeModules ? libraryComponentDirs : userComponentDirsAlso applies to: 101-104
97-99: Windows paths: default components dir regex misses backslashes.The warning may fire for default dirs on Windows. Make the regex path-separator agnostic.
-const DEFAULT_COMPONENTS_DIRS_RE = /\/components(?:\/(?:global|islands))?$/ +const DEFAULT_COMPONENTS_DIRS_RE = /[\\/]components(?:[\\/](?:global|islands))?$/
272-287: Docstring says “shallowest first” but comparator sorts “deepest first”.Either flip the comparator or fix the doc. I suggest updating the docs to reflect the current (and usually desired) “deepest first”.
- * Each resulting entry has its `path` resolved via `resolveAlias` and `cwd`, receives a `priority` from `options.priority` (default 0), and entries without a `path` are skipped. The final list is sorted by path depth (shallowest first). + * Each resulting entry has its `path` resolved via `resolveAlias` and `cwd`, receives a `priority` from `options.priority` (default 0), and entries without a `path` are skipped. The final list is sorted by path depth (deepest first).Also applies to: 321-322
289-291: Minor: avoid repeated sorts innormalizeDirsfor arrays.You can collect then sort once to cut redundant work (tiny perf win on large configs).
-if (Array.isArray(dir)) { - return dir.map(dir => normalizeDirs(dir, cwd, options)).flat().sort(compareDirByPathLength) -} +if (Array.isArray(dir)) { + const flat = dir.flatMap(d => normalizeDirs(d, cwd, options)) + return flat.sort(compareDirByPathLength) +}Also applies to: 308-321
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/components/module.ts(5 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/components/module.ts
🧠 Learnings (2)
📓 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.
📚 Learning: 2024-11-05T15:22:54.759Z
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.
Applied to files:
packages/nuxt/src/components/module.ts
🧬 Code graph analysis (1)
packages/nuxt/src/components/module.ts (6)
packages/nuxt/src/utils.ts (2)
isDirectory(10-12)logger(14-14)packages/kit/src/index.ts (3)
addBuildPlugin(20-20)resolveAlias(32-32)logger(36-36)packages/kit/src/build.ts (1)
addBuildPlugin(153-165)packages/nuxt/src/components/plugins/component-names.ts (1)
ComponentNamePlugin(16-68)packages/kit/src/resolve.ts (1)
resolveAlias(91-94)packages/vite/src/utils/transpile.ts (1)
transpile(11-28)
⏰ 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: codeql (javascript-typescript)
- GitHub Check: code
🔇 Additional comments (5)
packages/nuxt/src/components/module.ts (5)
24-35: Docstring forcompareDirByPathLengthlooks good.Clear, concise, and matches the implementation.
71-76: Layer dir aggregation vianormalizeDirsis a solid simplification.Single pass over
_layerswith per-layer priority assignment is clear and efficient.
120-123: User-before-library ordering is the right default.Preserves expected override semantics.
137-139: Generating metadata gated bymoduleOptions.generateMetadata: looks correct.Matches the updated setup signature and avoids accidental reads from stale names.
50-50: Verify rename: componentOptions → moduleOptions (repo‑wide)setup parameter renamed to moduleOptions in packages/nuxt/src/components/module.ts (≈ line 50). I couldn't verify automatically — confirm there are no remaining references to "componentOptions", that "components.generateMetadata" is used consistently, and that "exsolve" is declared/imported. Suggested checks (run and paste results):
- git grep -n -- 'componentOptions' || rg -n --hidden -S '\bcomponentOptions\b' -g '!/node_modules/'
- git grep -n -- 'generateMetadata' || rg -n --hidden -S 'components.generateMetadata|generateMetadata' -g '!/node_modules/'
- rg -n --hidden -S "from\s+['"]exsolve['"]" -g '!/node_modules/' || true
- jq -e '.dependencies.exsolve // .devDependencies.exsolve' package.json >/dev/null && echo 'exsolve in package.json' || echo 'exsolve missing'
| import type { Component, ComponentsDir, ComponentsOptions } from 'nuxt/schema' | ||
|
|
||
| const isPureObjectOrString = (val: any) => (!Array.isArray(val) && typeof val === 'object') || typeof val === 'string' | ||
| const isPureObjectOrString = (val: unknown): val is object | string => (!Array.isArray(val) && typeof val === 'object') || typeof val === 'string' |
There was a problem hiding this comment.
Fix the type guard: exclude null to avoid crashes when hooks mutate allDirs.
typeof null === 'object' so null currently passes the guard, leading to dirOptions = dir and dirOptions.path access throwing. Tighten the predicate.
-const isPureObjectOrString = (val: unknown): val is object | string => (!Array.isArray(val) && typeof val === 'object') || typeof val === 'string'
+const isPureObjectOrString = (val: unknown): val is Record<string, unknown> | string =>
+ typeof val === 'string' || (val !== null && typeof val === 'object' && !Array.isArray(val))📝 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.
| const isPureObjectOrString = (val: unknown): val is object | string => (!Array.isArray(val) && typeof val === 'object') || typeof val === 'string' | |
| const isPureObjectOrString = (val: unknown): val is Record<string, unknown> | string => | |
| typeof val === 'string' || (val !== null && typeof val === 'object' && !Array.isArray(val)) |
🤖 Prompt for AI Agents
In packages/nuxt/src/components/module.ts around line 21, the type guard is
treating null as an object because typeof null === 'object', which lets null
pass and later causes crashes when code reads dirOptions.path; update the
predicate to explicitly exclude null (e.g., check val !== null) so only actual
objects (non-null, non-array) or strings are accepted as valid values.
| } | ||
|
|
||
| // TODO: remove in Nuxt v5 | ||
| if (nuxt.options.experimental.normalizeComponentNames) { |
There was a problem hiding this comment.
is it about removing the flag or the feature ?
There was a problem hiding this comment.
it's about removing the flag - ie. hard-coding the behaviour
|
I ran into an issue with nuxt 4.1.2 where shadcn Form (using Vee Validate) is broken. I cant be 100% sure this PR is the cause, but I think it maybe. The issue is the |
|
@blowsie do you have a reproduction? 🙏 |
Not to hand, but if I get a minute I'll try and create one |
🔗 Linked issue
📚 Description
spotted while investing nuxt module hooks, this rewrites a lot of the logic in the components module to be more performant (preferring single loops rather than looping multiple times over the same array),
as well as to move thecomponents:dirshook intomodules:done. This should be non-breaking and much more performant but perhaps we can test this and revisit this as an experimental change if anyone on @nuxt/core has concerns.I've added types to an
anywhich highlighted a few edge cases we needed to handle, as well as some inaccurate types for thecomponentsoption in the Nuxt config.Additionally, I spotted we were sorting
components(if an array) by length but notcomponents.dirs(if an array). We're now treating both the same way.