fix(nuxt): ensure layer array-type config is merged in order #31507
Conversation
|
|
WalkthroughThe changes update the configuration and module handling logic in the Nuxt and kit packages. An enhanced merging strategy is introduced via a new ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/nuxt/test/layers-fixture/custom/module.ts (1)
1-1: Consider adding JSDoc documentationEven for test modules, adding documentation can help clarify the purpose and expected behaviour of this module. This is especially useful for other developers who may be working with these test fixtures.
+/** + * Custom test module for Nuxt layers functionality + */ export default function custom () {}packages/nuxt/src/core/utils/index.ts (1)
4-6: Consider using a Set for better performanceThe current implementation has O(n²) time complexity because
indexOfperforms a linear search for each element. For larger arrays, this could be inefficient. Consider using a Set for O(n) performance, similar to theuniqueByfunction below.export function unique<T> (array: readonly T[]): T[] { - return array.filter((value, index) => array.indexOf(value) === index) + return [...new Set(array)] }packages/nuxt/test/layers-fixture/custom/nuxt.config.ts (1)
2-2: Consider using path utilities for cross-platform compatibilityWhile the current implementation works for test fixtures, using string concatenation for paths can cause issues on different operating systems. For better cross-platform compatibility, consider using a path utility.
- modules: [import.meta.dirname + '/module'], + modules: [import.meta.dirname + '/module'.replace(/\//g, path.sep)],Note that you would need to import the
pathmodule at the top of the file:import path from 'path'packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts (1)
2-2: Consider extracting the inline module functionFor better readability and maintainability, consider extracting the inline function definition to a separate constant or file, even in test fixtures.
+function layer() {} + export default defineNuxtConfig({ - modules: [function layer () {}], + modules: [layer], css: ['duplicate.css', 'auto.css'], array: ['test'], })packages/kit/src/loader/config.ts (1)
51-51: Consider using undefined assignment instead of delete operator.The delete operator can have performance implications. Consider using an undefined assignment instead.
-delete globalSelf.defineNuxtConfig +globalSelf.defineNuxtConfig = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/kit/src/loader/config.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(3 hunks)packages/nuxt/src/core/utils/index.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/nuxt/src/core/nuxt.ts (3)
packages/kit/src/resolve.ts (1)
resolveAlias(74-77)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)packages/nuxt/src/core/utils/index.ts (1)
unique(4-6)
🪛 Biome (1.9.4)
packages/kit/src/loader/config.ts
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (13)
packages/kit/src/loader/config.ts (4)
9-9: Update to defu import for extended functionality.The change imports
createDefuin addition to the existingdefuimport. This extension enables custom merging strategies for configuration values.
20-32: Well-implemented custom merger function for array handling.This new custom merger function addresses the core issue with array values in configuration by:
- Concatenating arrays when merging configurations from different layers
- Supporting function-based overrides that can modify or replace array values
This implementation elegantly solves issue #25719 mentioned in the PR objectives, allowing users to completely override array values by returning an empty array.
39-41: Improved global variable handling.Using a local variable before defining and deleting the global function improves code clarity and makes the intent more explicit.
48-48: Added custom merger function to loadConfig with clear TODO.The merger function is properly integrated into the configuration loading process. The TODO comment clearly indicates the need for type improvements in the c12 library.
packages/nuxt/test/load-nuxt.test.ts (3)
63-63: Appropriately updated test expectations to include duplicate.css.This change correctly updates the test expectations to match the new behavior where duplicate CSS entries are preserved in the initial loading but later deduplicated using the
uniquefunction ininitNuxt.
111-126: Added test to verify correct module order from layers.This test ensures that modules from layers are correctly ordered in the final configuration, validating the change to module registration logic in the
initNuxtfunction. The test correctly verifies that the modules appear in the expected order: layer, custom module, project, and css.
128-138: Added test to verify array value overrides.This test validates the new ability to override array values in the configuration by using functions, specifically checking that the
arrayproperty in the configuration has been properly overridden with the value from the project layer.packages/nuxt/src/core/nuxt.ts (3)
56-56: Added import for the unique function.Importing the
uniquefunction for deduplicating CSS entries is appropriate and ensures proper code organization.
489-516: Improved module registration logic.The restructured code properly handles module registration in the correct order:
- First processes modules from reversed layers to maintain proper precedence
- Resolves module paths correctly
- Adds modules to the specifiedModules set to prevent duplication
This addresses the core issue mentioned in the PR objectives regarding the incorrect order of array values in configuration.
661-661: Added deduplication of CSS entries.Using the
uniquefunction to deduplicate CSS entries after all modules have been processed ensures that the final CSS list is clean while still maintaining the correct order of entries from layers.packages/nuxt/test/layers-fixture/nuxt.config.ts (3)
4-5: Added named functions for better readability.Naming the functions
projectandcssimproves code readability and makes it clear what each module function is responsible for.
11-11: Added function that returns an array for testing override functionality.This implementation demonstrates the new ability to override array values by returning a function that produces a new array. This is a key feature that addresses the PR objectives for allowing complete overrides of array values.
14-18: Added type definition for the new array property.The type declaration properly extends the NuxtConfig interface to include the new array property, which can be either an array of strings or a function returning an array of strings. This ensures proper TypeScript support for the new feature.
@nuxt/kit
nuxt
@nuxt/schema
@nuxt/rspack-builder
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #31507 will not alter performanceComparing Summary
|
9e92d9b to
c19e702
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/kit/src/loader/config.ts (2)
30-30: Consider using the new merger here as well.You're using the standard
defufor mergingopts.overridesinstead of the custommergerfunction you've created. This might lead to inconsistent merging behavior.- opts.overrides = defu(opts.overrides, { _extends: localLayers }) + opts.overrides = merger(opts.overrides, { _extends: localLayers })
44-44: Potential performance issue with delete operator.Using the
deleteoperator can impact performance as noted by the static analysis tool. Consider assigning undefined instead.- delete globalSelf.defineNuxtConfig + globalSelf.defineNuxtConfig = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/kit/src/loader/config.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(2 hunks)packages/nuxt/test/layers-fixture/custom/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/nuxt/test/layers-fixture/custom/module.ts
- packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts
- packages/nuxt/test/layers-fixture/custom/nuxt.config.ts
- packages/nuxt/test/load-nuxt.test.ts
- packages/nuxt/test/layers-fixture/nuxt.config.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/nuxt/src/core/nuxt.ts (2)
packages/kit/src/resolve.ts (1)
resolveAlias(74-77)packages/kit/src/internal/esm.ts (1)
directoryToURL(13-15)
🪛 Biome (1.9.4)
packages/kit/src/loader/config.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
packages/kit/src/loader/config.ts (4)
9-9: Good addition of the createDefu import.The inclusion of
createDefuallows for creating a custom merging strategy for configuration values.
20-25: Well-implemented array concatenation merger.This custom merger function elegantly solves the issue mentioned in the PR objectives regarding the order of array values in configuration. When both the existing value and the new value are arrays, they are concatenated, preserving their order.
32-33: Good practice to use a local variable.Creating a local
globalSelfvariable improves code clarity and makes it easier to understand the scope of operations.
41-41: TODO comment for future improvement.The TODO comment appropriately flags a type issue with c12 that should be addressed in the future. This is good practice to document known issues.
packages/nuxt/src/core/nuxt.ts (2)
488-502: Well-structured module registration approach.The new approach organizes module registration by iterating over layers in reverse order, which ensures proper precedence. This implementation aligns with the PR objective of fixing the order of values in config when utilizing layers.
659-661:✅ Verification successful
CSS deduplication logic changed.
Your implementation now keeps the first occurrence of duplicate CSS entries rather than the last one. This differs from the previous behaviour where CSS added explicitly in a project had priority (as mentioned in previous comments).
Based on past review comments, this change in behaviour was discussed and appears to be intentional, but you should verify that this is the desired outcome for all use cases. Run this script to compare the order of CSS entries with the new implementation:
🏁 Script executed:
#!/bin/bash # Verify CSS deduplication behavior with the new implementation # Look for tests that might be affected by this change rg -A 5 "options.css" packages/nuxt/test/ # Check if there are any snapshots containing CSS entries rg -l "css:" packages/nuxt/test/Length of output: 1075
CSS Deduplication Behaviour Confirmed
The updated deduplication logic has been verified against the test snapshots. The tests in
packages/nuxt/test/load-nuxt.test.ts(and related layer fixtures) now expect the array to retain the first occurrence of duplicate CSS entries, which aligns with the new implementation. No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nuxt/test/load-nuxt.test.ts (1)
111-126: New test case ensures module order preservationThis test case correctly verifies that module ordering is maintained when loading Nuxt from a layered configuration, which aligns with the PR's objective of fixing the order of array values in the configuration.
However, I noticed that the inline snapshot contains an absolute path:
"/home/runner/work/nuxt/nuxt/packages/nuxt/test/layers-fixture/custom/module"This may cause test failures if run in different environments where the absolute path structure differs.
Consider normalising this path in the test assertions to make it environment-independent:
- const modules = nuxt.options.modules.map(item => typeof item === 'function' ? item.name : item) + const modules = nuxt.options.modules.map(item => { + if (typeof item === 'function') return item.name + // Convert absolute paths to relative or normalised format + if (typeof item === 'string' && item.includes('/layers-fixture/')) { + return item.split('/layers-fixture/')[1] + } + return item + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/test/layers-fixture/nuxt.config.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: code
13baf7f to
b505495
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kit/src/loader/config.ts (1)
32-44: Consider replacing the delete operator with an undefined assignment.The static analysis tool flagged performance concerns with the
deleteoperator. While this is unlikely to be a bottleneck as it's only called once during initialisation, it might be worth considering the recommended fix.- delete globalSelf.defineNuxtConfig + globalSelf.defineNuxtConfig = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/kit/src/loader/config.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(2 hunks)packages/nuxt/test/layers-fixture/custom/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts
- packages/nuxt/test/layers-fixture/custom/module.ts
- packages/nuxt/test/layers-fixture/custom/nuxt.config.ts
- packages/nuxt/test/layers-fixture/nuxt.config.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/kit/src/loader/config.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (7)
packages/nuxt/test/load-nuxt.test.ts (1)
111-126: Good test addition to ensure module order is preserved.This test ensures that modules from layers are registered in the expected order after loading a Nuxt instance. It properly validates the changes made to the module registration logic in
packages/nuxt/src/core/nuxt.tswhere modules are now processed in reverse order from the layers.packages/kit/src/loader/config.ts (4)
9-9: Good update to include createDefu for enhanced merging logic.The addition of
createDefufrom the defu library is a good choice for implementing custom array concatenation behaviour for the configuration merger.
20-25: Well implemented array concatenation logic.This new
mergerfunction provides a clean solution for the PR's objective of fixing array value order in configurations. It checks if both values are arrays and concatenates them, preserving the order from each layer.
30-30: Using standard defu for this override is correct.For the
_extendsproperty, using the standarddefufunction instead of the custommergeris appropriate since we don't want to concatenate arrays in this specific case.
41-41: This TODO comment is reasonable for now.The comment acknowledges that there's a type issue in c12 that should be fixed, and the current cast to
anyis a temporary workaround. It's good to document this for future cleanup.packages/nuxt/src/core/nuxt.ts (2)
488-502: Great implementation of layer-based module loading.This new approach iterates through layers in reverse order and registers modules from each layer, ensuring that the order of modules is preserved according to the layer structure. This fixes the issue mentioned in the PR objectives about incorrect order of array values in configuration.
659-662: Well-implemented CSS deduplication logic.The CSS deduplication logic correctly filters out duplicates while preserving the order, ensuring that the first occurrence of each CSS entry is kept while subsequent duplicates are removed. This aligns with the layer CSS test changes.
a36054b to
19361dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kit/src/loader/config.ts (1)
44-44: Consider replacing delete operator for better performance.The
deleteoperator can impact performance as noted by the static analysis tool. Consider using an undefined assignment instead.- delete globalSelf.defineNuxtConfig + globalSelf.defineNuxtConfig = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/kit/src/loader/config.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(2 hunks)packages/nuxt/test/layers-fixture/custom/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/nuxt/test/layers-fixture/custom/module.ts
- packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts
- packages/nuxt/test/layers-fixture/custom/nuxt.config.ts
- packages/nuxt/test/layers-fixture/nuxt.config.ts
- packages/nuxt/test/load-nuxt.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/kit/src/loader/config.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (3)
packages/nuxt/src/core/nuxt.ts (2)
488-502: Great improvement for module order handling!This change ensures correct ordering of modules from different layers by iterating through layers in reverse order, which aligns perfectly with the PR objective of fixing the order of values in configuration when using layers.
659-662: Effective implementation of CSS deduplication.The updated approach to remove duplicate CSS entries maintains the order while ensuring only the first occurrence of each value is preserved. This is consistent with the discussion in the PR comments about handling CSS duplicates.
packages/kit/src/loader/config.ts (1)
20-25: Well-implemented array merging strategy.The new
mergerfunction elegantly handles array concatenation for configuration values, which is crucial for maintaining the correct order of arrays when merging configurations from different layers.
19361dd to
f5b25a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nuxt/src/core/nuxt.ts (1)
620-622: 🛠️ Refactor suggestionEnsure CSS order prioritises project CSS over layer CSS.
The CSS deduplication logic is using the first occurrence of each value, but based on previous discussions, project CSS should have priority over layer CSS, meaning the last occurrence should be preserved.
- nuxt.options.css = nuxt.options.css - .filter((value, index, array) => !array.includes(value, index + 1)) + nuxt.options.css = nuxt.options.css + .filter((value, index, array) => array.lastIndexOf(value) === index)This change will keep the last occurrence of each CSS entry, giving priority to project CSS over layer CSS.
🧹 Nitpick comments (2)
packages/kit/src/loader/config.ts (2)
30-30: Ensure consistent merging strategy.For consistency, consider using the new
mergerfunction here instead ofdefu.- opts.overrides = defu(opts.overrides, { _extends: localLayers }) + opts.overrides = merger(opts.overrides, { _extends: localLayers })
44-44: Consider using undefined assignment instead of delete operator.The delete operator can impact performance according to the static analysis tool.
- delete globalSelf.defineNuxtConfig + globalSelf.defineNuxtConfig = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/kit/src/loader/config.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(4 hunks)packages/nuxt/test/layers-fixture/custom/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/modules/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/module.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/modules/module.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/module.ts(1 hunks)packages/nuxt/test/layers-fixture/modules/module.ts(1 hunks)packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/nuxt/test/layers-fixture/module.ts
- packages/nuxt/test/layers-fixture/modules/module.ts
- packages/nuxt/test/layers-fixture/layers/auto/module.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nuxt/test/layers-fixture/custom/nuxt.config.ts
- packages/nuxt/test/load-nuxt.test.ts
- packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts
- packages/nuxt/test/layers-fixture/nuxt.config.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/nuxt/src/core/nuxt.ts (3)
packages/kit/src/index.ts (4)
resolveModule(35-35)resolveAlias(28-28)directoryToURL(35-35)resolveFiles(28-28)packages/kit/src/internal/esm.ts (2)
resolveModule(34-39)directoryToURL(13-15)packages/kit/src/resolve.ts (2)
resolveAlias(74-77)resolveFiles(238-247)
🪛 Biome (1.9.4)
packages/kit/src/loader/config.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (12)
packages/nuxt/test/layers-fixture/layers/auto/modules/module.ts (1)
1-1: Looks good - appropriate test module structure.This empty module function follows the expected pattern for Nuxt module testing fixtures. The clear naming convention (
autoLayerAutoModule) appropriately indicates its purpose within the layer testing context.packages/nuxt/test/layers-fixture/custom/module.ts (1)
1-1: Module implementation looks goodThe function is well-named and follows the proper export syntax for a Nuxt module. The empty implementation is perfectly acceptable for a test fixture module, as it appears to be used primarily for testing module registration and ordering logic rather than actual functionality.
packages/nuxt/test/layers-fixture/custom/modules/module.ts (1)
1-1: Auto-module implementation looks goodThe function naming with "Auto" in
customLayerAutoModulesuggests this module is designed for automatic discovery and loading when placed in themodulesdirectory. This follows Nuxt module conventions and is appropriately implemented with an empty function body for testing purposes.packages/nuxt/src/core/nuxt.ts (4)
484-487: Great refactoring to use the new resolveModules function.This changes consolidates module resolution into a dedicated function, making the code more maintainable. The destructuring assignment makes it clear what parts of the result are being used.
609-611: Clean iteration over resolved modules.This change eliminates the previous array iteration complexity and works well with the new structure returned by
resolveModules.
941-970: Well-implemented module resolution function.This utility function properly resolves a module definition to a module and options pair, handling both string identifiers and direct module references. It correctly uses
resolveAliasandresolveModulePathto find the actual module path.
972-1020: Great implementation of module resolution logic.The
resolveModulesfunction properly handles the order of module resolution:
- Processes layers in reverse order so that project modules have priority
- Registers modules defined in each layer's config
- Registers modules from layer directories
- Handles private modules and modules added after loading config
This implementation helps fix the issue with the incorrect order of array values in the configuration when utilizing layers.
packages/kit/src/loader/config.ts (5)
9-9: Good addition of createDefu import.Adding the
createDefuimport is necessary for creating the custom merger function.
20-25: Excellent addition of custom array merging strategy.The
mergerfunction elegantly handles array concatenation when both the target and source values are arrays. This is important for preserving the order of configuration values across layers.
32-33: Cleaner global context reference.Creating a local
globalSelfvariable improves code readability and consistency.
38-39: Correctly configured extension keys.The extend configuration with multiple extension keys allows for flexible configuration inheritance.
41-42: Successfully integrated custom merger.The custom merger is now applied to the configuration loading process. The TODO comment appropriately flags a type issue to be addressed in the future.
1236fb0 to
1946dba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kit/src/loader/config.ts (1)
32-33: Better approach for global object handling.Using a local variable for the global object improves code clarity and maintainability. However, there's a linting warning about using the delete operator which could impact performance.
Consider using an assignment to undefined instead of delete for better performance:
- const globalSelf = globalThis as any - globalSelf.defineNuxtConfig = (c: any) => c - // ... code ... - delete globalSelf.defineNuxtConfig + const globalSelf = globalThis as any + globalSelf.defineNuxtConfig = (c: any) => c + // ... code ... + globalSelf.defineNuxtConfig = undefinedAlso applies to: 44-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/kit/src/loader/config.ts(2 hunks)packages/nuxt/src/core/nuxt.ts(4 hunks)packages/nuxt/test/layers-fixture/custom/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/modules/module.ts(1 hunks)packages/nuxt/test/layers-fixture/custom/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/module.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/modules/module.ts(1 hunks)packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts(1 hunks)packages/nuxt/test/layers-fixture/module.ts(1 hunks)packages/nuxt/test/layers-fixture/modules/module.ts(1 hunks)packages/nuxt/test/layers-fixture/nuxt.config.ts(1 hunks)packages/nuxt/test/load-nuxt.test.ts(2 hunks)packages/nuxt/test/shared-dir-config.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/nuxt/test/layers-fixture/module.ts
- packages/nuxt/test/layers-fixture/modules/module.ts
- packages/nuxt/test/layers-fixture/layers/auto/module.ts
- packages/nuxt/test/layers-fixture/custom/module.ts
- packages/nuxt/test/layers-fixture/layers/auto/modules/module.ts
- packages/nuxt/test/layers-fixture/custom/modules/module.ts
- packages/nuxt/test/load-nuxt.test.ts
- packages/nuxt/test/layers-fixture/custom/nuxt.config.ts
- packages/nuxt/test/shared-dir-config.test.ts
- packages/nuxt/test/layers-fixture/nuxt.config.ts
- packages/nuxt/test/layers-fixture/layers/auto/nuxt.config.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/nuxt/src/core/nuxt.ts (3)
packages/kit/src/index.ts (4)
resolveModule(35-35)resolveAlias(28-28)directoryToURL(35-35)resolveFiles(28-28)packages/kit/src/internal/esm.ts (2)
resolveModule(34-39)directoryToURL(13-15)packages/kit/src/resolve.ts (2)
resolveAlias(74-77)resolveFiles(238-247)
🪛 Biome (1.9.4)
packages/kit/src/loader/config.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (9)
packages/nuxt/src/core/nuxt.ts (5)
484-487: Good refactoring to use the new resolveModules function.The refactoring improves code structure by separating module resolution logic into a dedicated function. This makes the code more maintainable and easier to understand.
609-611: Improved module installation logic.The iteration over modules is now clearer and more straightforward, using a destructured map entry approach.
619-622: CSS handling keeps first occurrence instead of last.The filtering logic keeps the first occurrence of duplicate CSS entries, which changes the previous behaviour where project CSS would override layer CSS. This appears intentional based on the PR discussion.
941-970: Well-implemented resolveModule function.The new function handles various module definition formats and properly resolves module paths. The approach is robust and accounts for different module formats and resolution scenarios.
972-1020: Thorough implementation of resolveModules function.This function handles modules from multiple sources:
- Modules defined in layer configurations
- Modules from layer directories
- Private modules and modules added after loading config
The module resolution happens in a logical order, ensuring that the correct precedence is maintained.
packages/kit/src/loader/config.ts (4)
9-9: Added import for createDefu.This import enables the creation of a custom merge strategy for configuration objects.
20-25: Good implementation of array concatenation merger.The custom merger function ensures that when both values are arrays, they are concatenated rather than replaced. This addresses the core issue of preserving array values in configuration when using layers.
30-30: Improved options overrides handling with defu.Using defu to merge the overrides with existing options ensures that all properties are properly combined.
41-41: Using custom merger for config loading.The merger is correctly applied to the loadConfig call, enabling array concatenation in configuration loading. The TODO comment about fixing the type in c12 is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nuxt/src/core/nuxt.ts (1)
970-1027: Comprehensive module resolution with proper layer ordering.The
resolveModulesfunction correctly processes layers in reverse order, ensuring project-level modules take precedence. It also handles both explicitly defined modules in config and automatically discovered modules from the layers' module directories.One note: The function could potentially benefit from some error handling for edge cases where module resolution fails.
Consider adding error handling for cases where module resolution might fail:
async function resolveModules (nuxt: Nuxt) { const modules = new Map<string | NuxtModule, Record<string, any>>() const paths = new Set<string>() const resolvedModulePaths = new Set<string>() // Loop layers in reverse order, so that the extends are loaded first and project is the last const configs = nuxt.options._layers.map(layer => layer.config).reverse() for (const config of configs) { // First register modules defined in layer's config const definedModules = config.modules ?? [] for (const module of definedModules) { - const resolvedModule = resolveModule(module, nuxt) + try { + const resolvedModule = resolveModule(module, nuxt) + if (resolvedModule && (!resolvedModule.resolvedPath || !resolvedModulePaths.has(resolvedModule.resolvedPath))) { + modules.set(resolvedModule.module, resolvedModule.options) + const path = resolvedModule.resolvedPath || typeof resolvedModule.module + if (typeof path === 'string') { + resolvedModulePaths.add(path) + } + } + } catch (error) { + console.warn(`Failed to resolve module: ${typeof module === 'string' ? module : 'non-string module'}`, error) + } - if (resolvedModule && (!resolvedModule.resolvedPath || !resolvedModulePaths.has(resolvedModule.resolvedPath))) { - modules.set(resolvedModule.module, resolvedModule.options) - const path = resolvedModule.resolvedPath || typeof resolvedModule.module - if (typeof path === 'string') { - resolvedModulePaths.add(path) - } - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/core/nuxt.ts(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/nuxt/src/core/nuxt.ts (4)
packages/kit/src/index.ts (3)
resolveModule(35-35)resolveAlias(28-28)resolveFiles(28-28)packages/kit/src/internal/esm.ts (1)
resolveModule(34-39)packages/schema/src/types/nuxt.ts (1)
Nuxt(89-116)packages/kit/src/resolve.ts (2)
resolveAlias(74-77)resolveFiles(238-247)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (4)
packages/nuxt/src/core/nuxt.ts (4)
484-484: Clean refactoring to use the new resolveModules function.The code now uses a better structured approach by extracting module resolution logic into a dedicated function, making the code more maintainable and easier to reason about.
606-608: Simpler module installation loops through the resolved modules.This iteration approach is cleaner than the previous implementation, as it directly loops through the structured data returned by
resolveModules. This makes the code more concise and easier to follow.
617-619: New CSS deduplication logic to maintain order.The new code maintains CSS order by keeping the first occurrence of any CSS entry and filtering out duplicates that appear later in the array. This addresses the original issue where array-type configurations needed to be merged in the correct order.
938-968: Well-structured resolveModule function with proper module path resolution.This function handles different types of module definitions (strings, objects, arrays) and provides a standardized output format. The use of
resolveAliasandresolveModulePathwith appropriate options ensures robust module resolution across various scenarios.
|
Thanks for this nice PR ❤️ We rely on the path to generate types (which is why types were breaking in the fixture), so we can't fully resolve the path before hitting |
|
as it's possible people are relying on the previous order (flagged up in an ecosystem-ci run on nuxt-modules/i18n), we're not including this in v3.16.2 release until we have a better signal on how likely it is this would affect people. |
🔗 Linked issue
📚 Description
This PR fixes the the incorrect order of array values in config when using layers.