fix(kit): ensure local layers are prioritised alphabetically #33030
Conversation
|
|
danielroe
requested review from
a team,
antfu and
huang-julien
and removed request for
a team
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 1
🧹 Nitpick comments (10)
packages/kit/test/layer-fixture/custom-layers/a/nuxt.config.ts (1)
1-1: UsedefineNuxtConfigfor fixture consistency and type inferenceOther fixtures (e.g.
layers/d/nuxt.config.ts) usedefineNuxtConfig({}). Adopting the same here keeps fixtures consistent and enables Nuxt’s config typing.-export default {} +export default defineNuxtConfig({})packages/kit/src/loader/config.ts (1)
29-34: Make local layer ordering fully deterministic across locales and caseUsing
localeComparewithout options can produce locale-dependent orderings and different results for mixed case or diacritics. Since layer directory names are file-system identifiers, a simple, locale-agnostic compare is safer. Also, consider a more robust trailing-slash removal.- const localLayers = (await glob('layers/*', { - onlyDirectories: true, cwd: opts.cwd || process.cwd(), - })) - .map((d: string) => d.endsWith('/') ? d.substring(0, d.length - 1) : d) - .sort((a, b) => b.localeCompare(a)) + const localLayers = (await glob('layers/*', { + onlyDirectories: true, + cwd: opts.cwd || process.cwd(), + })) + // Ensure consistent form like `layers/foo` (no trailing slash) + .map((d: string) => d.replace(/\/+$/, '')) + // Stable, locale-agnostic, case-insensitive descending order (Z before A) + .sort((a, b) => { + const aa = a.toLowerCase() + const bb = b.toLowerCase() + if (aa === bb) { return 0 } + return aa > bb ? -1 : 1 + })If you’d prefer using a utility,
withoutTrailingSlash(fromufo) could replace the regex, but the current import set already includesufoonly forwithTrailingSlash. Keeping this self-contained avoids extra imports.docs/2.guide/3.going-further/7.layers.md (1)
73-76: Clarify “alphabetical order” wording to “descending alphabetical order”“Alphabetical order (Z overrides A)” is contradictory. Say “descending alphabetical order (Z overrides A)” to remove ambiguity.
-2. **Auto-scanned local layers** from `~~/layers` directory in alphabetical order (Z overrides A) +2. **Auto-scanned local layers** from `~~/layers` directory in descending alphabetical order (Z overrides A)docs/1.getting-started/14.layers.md (2)
82-84: Clarify “alphabetical order” wording to “descending alphabetical order”Match the implementation: local layers are sorted so Z overrides A.
-2. **Auto-scanned local layers** from `~~/layers` directory in alphabetical order (Z overrides A) +2. **Auto-scanned local layers** from `~~/layers` directory in descending alphabetical order (Z overrides A)
86-95: Reinforce project priority in the exampleTo avoid misinterpretation, mirror the narrative that the project has the highest priority by explicitly stating the effective order (left to right overrides).
export default defineNuxtConfig({ extends: [ - '../base', // Highest priority (among extends) - '@my-themes/awesome', // Medium priority - 'github:my-themes/awesome#v1', // Lower priority + '../base', // Highest priority among extends + '@my-themes/awesome', // Then this + 'github:my-themes/awesome#v1', // Then this ] - // Your project has the highest priority + // Your project has the highest priority overall (it overrides any layer) })Optionally add a one-liner right after the block:
-This means if multiple layers define the same component, configuration, or file, the one with higher priority will be used. +This means if multiple layers define the same item, the one with higher priority (your project first, then extends order, then local layers Z→A) will be used.packages/kit/test/layer-fixture/custom-layers/b/nuxt.config.ts (1)
1-1: Fixture is fine; optional consistency tweak with defineNuxtConfigThis minimal layer config is OK for the test. For consistency with other fixtures and better type inference, you could wrap it with defineNuxtConfig.
-export default {} +export default defineNuxtConfig({})packages/kit/test/layer-fixture/nuxt.config.ts (1)
2-2: Extends order matches intended precedence; add a clarifying commentPlacing './custom-layers/b' before './custom-layers/a' reflects “first overrides second”. Consider documenting this inline to avoid confusion when reading the fixture.
- extends: ['./custom-layers/b', './custom-layers/a'], + // Earlier entries override later ones + extends: ['./custom-layers/b', './custom-layers/a'],packages/kit/test/load-nuxt-config.spec.ts (3)
13-26: Snapshot looks good; consider reducing brittleness by checking presence rather than full mapInline snapshots of the entire alias object can become brittle if unrelated aliases are added later. If you only care that local-layer aliases are present and correctly rooted, assert on those keys specifically.
- expect(config.alias).toMatchInlineSnapshot(` - { - "#build": "<rootDir>/.nuxt/", - "#internal/nuxt/paths": "<rootDir>/.nuxt/paths.mjs", - "#layers/c": "<rootDir>/layers/c/", - "#layers/d": "<rootDir>/layers/d/", - "#layers/layer-fixture": "<rootDir>/", - "#shared": "<rootDir>/shared/", - "@": "<rootDir>/", - "@@": "<rootDir>/", - "~": "<rootDir>/", - "~~": "<rootDir>/", - } - `) + expect(config.alias).toMatchObject({ + '#layers/c': '<rootDir>/layers/c/', + '#layers/d': '<rootDir>/layers/d/', + '#layers/layer-fixture': '<rootDir>/' + })
29-36: Rename test and clarify comment to explicitly state descending order and array semantics“Alphabetical order” can be read as A→Z; here you mean Z→A. Also the bullet points claim “priority list” but the array you assert is the assembled order of config._layers (not necessarily the merge/override precedence). Clarify to avoid reader confusion.
- it('should respect alphabetical order of local layers', async () => { + it('should order local layers in descending alphabetical order (Z before A)', async () => { @@ - // priority list - // 1. layers in nuxt.config (first overrides second) - // 2. then local layers in alphabetical order (Z overrides A) - // 3. local project overrides + // Assembled order of config._layers (not override precedence): + // 1) local project (root) + // 2) auto-scanned local layers in descending alphabetical order (Z → A) + // 3) layers declared in nuxt.config extends, in declaration order (first overrides second)
36-36: Prefer meta.name where available for stabilityLocal layers and the project layer have meta.name set; using it reduces coupling to directory naming. Falls back to basename for layers without a meta name (e.g. custom-layers from extends).
- expect(config._layers.map(l => basename(l.cwd))).toMatchInlineSnapshot(` + const layerNames = config._layers.map(l => l.meta?.name ?? basename(l.cwd)) + expect(layerNames).toMatchInlineSnapshot(`
📜 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 (8)
docs/1.getting-started/14.layers.md(1 hunks)docs/2.guide/3.going-further/7.layers.md(1 hunks)packages/kit/src/loader/config.ts(1 hunks)packages/kit/test/layer-fixture/custom-layers/a/nuxt.config.ts(1 hunks)packages/kit/test/layer-fixture/custom-layers/b/nuxt.config.ts(1 hunks)packages/kit/test/layer-fixture/layers/d/nuxt.config.ts(1 hunks)packages/kit/test/layer-fixture/nuxt.config.ts(1 hunks)packages/kit/test/load-nuxt-config.spec.ts(3 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:
packages/kit/src/loader/config.tspackages/kit/test/layer-fixture/custom-layers/b/nuxt.config.tspackages/kit/test/layer-fixture/custom-layers/a/nuxt.config.tspackages/kit/test/layer-fixture/nuxt.config.tspackages/kit/test/layer-fixture/layers/d/nuxt.config.tspackages/kit/test/load-nuxt-config.spec.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/kit/test/load-nuxt-config.spec.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/kit/src/loader/config.ts
🧬 Code graph analysis (1)
packages/kit/test/load-nuxt-config.spec.ts (1)
packages/kit/src/loader/config.ts (1)
loadNuxtConfig(27-125)
🪛 LanguageTool
docs/1.getting-started/14.layers.md
[uncategorized] ~97-~97: Did you mean “is”?
Context: ...the highest priority }) ``` This means if multiple layers define the same compone...
(IF_IS_PREMIUM)
[uncategorized] ~98-~98: Loose punctuation mark.
Context: ...one with higher priority will be used. ::read-more{to="/docs/guide/going-further...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ 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
🔇 Additional comments (3)
packages/kit/test/layer-fixture/layers/d/nuxt.config.ts (1)
1-1: LGTM — minimal fixture is correctThe config is intentionally minimal and matches usage in tests. No changes needed.
packages/kit/src/loader/config.ts (1)
29-34: Confirm precedence interaction:_extends(local) vsextends(user-configured)This change feeds auto-scanned local layers via
_extends. WithextendKey: ['theme', '_extends', 'extends'], ensure the intended overall priority is: userextends(earlier wins) > auto-scanned local layers (Z before A) > project. If the order ofextendKeyaffects merge precedence (in c12), we should verify it still yields the documented priority after this PR.You can quickly inspect the resolved
_layersorder in the test fixture by logging during the test run or by adding a temporary assertion that checks the project layer is first and local layers follow in the expected desc alphabetical order. If helpful, I can propose an additional test asserting the combined order when bothextendsand local layers are present.packages/kit/test/load-nuxt-config.spec.ts (1)
4-4: Good call using pathe.basename for cross-platform behaviourUsing pathe avoids Windows path edge cases when deriving layer names from cwd.
| }) | ||
| ``` | ||
|
|
||
| If you also have auto-scanned layers like `~~/layers/a` and `~~/layers/z`, the complete override order would be: `base` > `theme` > `custom` > `z` > `a` > your project. |
There was a problem hiding this comment.
Fix priority example: project should appear first in override order
The paragraph says the project has the highest priority, but the example lists it last, which implies the opposite. Place “your project” first (leftmost) if “>” means “overrides”.
-If you also have auto-scanned layers like `~~/layers/a` and `~~/layers/z`, the complete override order would be: `base` > `theme` > `custom` > `z` > `a` > your project.
+If you also have auto-scanned layers like `~~/layers/a` and `~~/layers/z`, the complete override order would be: your project > `base` > `theme` > `custom` > `z` > `a`.📝 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.
| If you also have auto-scanned layers like `~~/layers/a` and `~~/layers/z`, the complete override order would be: `base` > `theme` > `custom` > `z` > `a` > your project. | |
| If you also have auto-scanned layers like `~~/layers/a` and `~~/layers/z`, the complete override order would be: your project > `base` > `theme` > `custom` > `z` > `a`. |
🤖 Prompt for AI Agents
In docs/2.guide/3.going-further/7.layers.md around line 90, the example override
order contradicts the text: it claims "your project" has highest priority but
lists it last; update the example so "your project" appears first (leftmost) in
the sequence and ensure the comparator ">" is used consistently to mean
"overrides" (e.g., your project > custom > theme > base > a > z).
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33030 will not alter performanceComparing Summary
|
danielroe
changed the title
2 participants
🔗 Linked issue
resolves #33017
📚 Description
this addresses unintuitive behaviour, where alphabetically earlier (A) layers in
~~/layerswere overriding 'later' (Z) layers.in a way this was consistent with 'earlier' layers in the
extendsarray overriding later ones, but at the same time this is confusing and we should probably have Z override A.I've also updated the docs.