fix(nuxt): handle arrays in app config correctly during HMR #33555
Conversation
|
|
WalkthroughThis pull request modifies the deep object merging logic in the Nuxt app configuration system. Changes to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The review requires careful examination of the logic changes in Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
![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 (1)
packages/nuxt/src/app/config.ts (1)
48-52: Type mismatch handling correctly addresses array↔object transitions.The explicit check for array type mismatches ensures that switching between arrays and objects during HMR works correctly, preventing type errors and ensuring proper replacement semantics.
Consider adding a test case for primitive→non-array object transitions (e.g., changing
count: 42tocount: { value: 42 }). Whilst less common in app config, the current logic won't correctly replace a primitive with a non-array object because Line 51's??=won't overwrite the existing primitive value. If this scenario is relevant, you could add:if (isPojoOrArray(val)) { const defaultVal = Array.isArray(val) ? [] : {} - if (Array.isArray(obj[key]) !== Array.isArray(val)) { + if (Array.isArray(obj[key]) !== Array.isArray(val) || !isPojoOrArray(obj[key])) { obj[key] = defaultVal } else { obj[key] ??= defaultVal }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/app/config.ts(2 hunks)packages/nuxt/test/app-config.test.ts(1 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/nuxt/test/app-config.test.tspackages/nuxt/src/app/config.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/nuxt/test/app-config.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write unit tests for core functionality using `vitest`
Applied to files:
packages/nuxt/test/app-config.test.ts
🧬 Code graph analysis (1)
packages/nuxt/test/app-config.test.ts (1)
packages/nuxt/src/app/config.ts (1)
_replaceAppConfig(67-72)
⏰ 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). (19)
- GitHub Check: test-fixtures (windows-latest, built, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: test-size
- GitHub Check: test-benchmark
- GitHub Check: code
🔇 Additional comments (4)
packages/nuxt/src/app/config.ts (1)
24-28: LGTM! Array handling prevents gaps during HMR.The new array-specific logic correctly addresses the issue where removing properties could leave gaps in arrays. By clearing and re-populating the array, you ensure the array remains contiguous and maintains its reference.
Note: Array elements are shallow-copied (spread operator), so nested objects within array elements share references with
newObj. This is consistent with the overall replacement semantics of_replaceAppConfig.packages/nuxt/test/app-config.test.ts (3)
1-23: LGTM! Well-structured test setup.The test setup correctly mocks
useNuxtAppand provides a clean state before each test. The mock structure aligns with the expected AppConfig shape.
25-99: Comprehensive test coverage for the HMR fix.These test cases thoroughly validate the fix for the reported issues:
- Property removal without leaving gaps
- Array updates and element order preservation
- Array↔object type switching (crucial for the bug fix)
The tests directly address the problems described in issue #33553.
101-123: Reference preservation tests validate efficient merging.These tests ensure that object references are preserved when content is unchanged, which is important for:
- Performance (avoiding unnecessary object creation)
- Reactivity systems (preventing unnecessary re-renders)
- Maintaining Vue's reactive tracking
The tests correctly verify that both object properties and array elements maintain their references when appropriate.
CodSpeed Performance ReportMerging #33555 will not alter performanceComparing Summary
|
|
❤️ |
2 participants
🔗 Linked issue
fix #33553
📚 Description
This fixes several issues with app config merging during HMR. Previously, the implementation would leave gaps in arrays where properties were removed, or fail to correctly replace arrays with objects and vice versa.