fix: zustand deep merge entities #404
Conversation
WalkthroughThe pull request introduces a new test case for the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant DS as DojoStore
participant ME as mergeEntities
participant DM as deepMerge
T->>DS: Call mergeEntities(updatedEntity)
DS->>ME: Process merging logic
ME->>DM: For each namespace, deep merge target and source
DM-->>ME: Return merged data
ME-->>DS: Update store with merged entity
DS-->>T: Confirm updated state
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 (1)
packages/sdk/src/state/zustand.ts (1)
94-125: Consider replacingObject.entries(...).forEachwith afor...ofloop.Performance-focused linters often advise using
for...ofinstead ofArray.prototype.forEach, especially in performance-critical paths or large data sets. Below is a possible adjustment (lines 98–118 shown briefly as an example):-Object.entries(entity.models).forEach(([namespace, namespaceModels]) => { - ... -}); +for (const [namespace, namespaceModels] of Object.entries(entity.models)) { + ... +}This avoids creating additional function scopes at each iteration step. If your data sizes are moderate, this may be a low-priority optimization, but it can still help align with best practices.
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-118: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 105-105: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sdk/src/__tests__/state.test.ts(1 hunks)packages/sdk/src/state/zustand.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/__tests__/state.test.ts
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts
[error] 25-25: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 27-27: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 98-118: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 105-105: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 116-116: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (2)
packages/sdk/src/__tests__/state.test.ts (1)
114-126: Great addition to test coverage for merging entities.
The newly introduced test thoroughly verifies that existing entities are properly updated without overwriting unrelated fields. This helps guard against regressions in nested property merges.🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts (1)
85-91: Straightforward check for new entity creation.
If an existing entity is not found, assigning the entire new entity is a clean approach that avoids partial merges and potential confusion.
packages/sdk/src/state/zustand.ts
Outdated
| function deepMerge<T extends SchemaType>( | ||
| target: ParsedEntity<T>["models"][keyof ParsedEntity<T>["models"]], | ||
| source: Record<string, any> | ||
| ): ParsedEntity<T>["models"][keyof ParsedEntity<T>["models"]] { | ||
| const result = { ...target } as Record<string, any>; | ||
|
|
||
| for (const key in source) { | ||
| if (Object.prototype.hasOwnProperty.call(source, key)) { | ||
| if ( | ||
| source[key] !== null && | ||
| typeof source[key] === "object" && | ||
| !Array.isArray(source[key]) | ||
| ) { | ||
| // If the property is an object in both source and target, recursively merge | ||
| if ( | ||
| key in target && | ||
| typeof target[key] === "object" && | ||
| !Array.isArray(target[key]) | ||
| ) { | ||
| result[key] = deepMerge(target[key], source[key]); | ||
| } else { | ||
| // If the key doesn't exist in target or isn't an object, just assign | ||
| result[key] = source[key]; | ||
| } | ||
| } else { | ||
| // For non-objects (primitives, arrays, null), just assign | ||
| result[key] = source[key]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any in deepMerge for stricter type safety.
Currently, the parameters source: Record<string, any> (line 25) and the cast to Record<string, any> (line 27) reduce type-checking rigor. If feasible, consider defining a more precise generic or mapped type for both source and result. This will help surface potential type mismatches earlier in development.
Below is an illustrative example of how you could approach tightening the types. Adjust as needed to align with your ParsedEntity<T> design:
-function deepMerge<T extends SchemaType>(
- target: ParsedEntity<T>["models"][keyof ParsedEntity<T>["models"]],
- source: Record<string, any>
-): ParsedEntity<T>["models"][keyof ParsedEntity<T>["models"]] {
- const result = { ...target } as Record<string, any>;
+function deepMerge<TObject extends object>(
+ target: TObject,
+ source: Partial<TObject>
+): TObject {
+ const result: TObject = { ...target };
for (const key in source) {
// ... rest of merging logic ...
}
return result;
}📝 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.
| function deepMerge<T extends SchemaType>( | |
| target: ParsedEntity<T>["models"][keyof ParsedEntity<T>["models"]], | |
| source: Record<string, any> | |
| ): ParsedEntity<T>["models"][keyof ParsedEntity<T>["models"]] { | |
| const result = { ...target } as Record<string, any>; | |
| for (const key in source) { | |
| if (Object.prototype.hasOwnProperty.call(source, key)) { | |
| if ( | |
| source[key] !== null && | |
| typeof source[key] === "object" && | |
| !Array.isArray(source[key]) | |
| ) { | |
| // If the property is an object in both source and target, recursively merge | |
| if ( | |
| key in target && | |
| typeof target[key] === "object" && | |
| !Array.isArray(target[key]) | |
| ) { | |
| result[key] = deepMerge(target[key], source[key]); | |
| } else { | |
| // If the key doesn't exist in target or isn't an object, just assign | |
| result[key] = source[key]; | |
| } | |
| } else { | |
| // For non-objects (primitives, arrays, null), just assign | |
| result[key] = source[key]; | |
| } | |
| } | |
| } | |
| return result; | |
| } | |
| function deepMerge<TObject extends object>( | |
| target: TObject, | |
| source: Partial<TObject> | |
| ): TObject { | |
| const result: TObject = { ...target }; | |
| for (const key in source) { | |
| if (Object.prototype.hasOwnProperty.call(source, key)) { | |
| if ( | |
| source[key] !== null && | |
| typeof source[key] === "object" && | |
| !Array.isArray(source[key]) | |
| ) { | |
| // If the property is an object in both source and target, recursively merge | |
| if ( | |
| key in target && | |
| typeof target[key] === "object" && | |
| !Array.isArray(target[key]) | |
| ) { | |
| result[key] = deepMerge(target[key], source[key] as any); | |
| } else { | |
| // If the key doesn't exist in target or isn't an object, just assign | |
| result[key] = source[key] as any; | |
| } | |
| } else { | |
| // For non-objects (primitives, arrays, null), just assign | |
| result[key] = source[key] as any; | |
| } | |
| } | |
| } | |
| return result; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 27-27: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
MartianGreed
force-pushed
the
fix/zustand-merge-entities
branch
from
f49e2af to
26d4dbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/state.test.ts (1)
123-123: Use a literal key directly instead of a string literal for better type safetyThe computed property access can be simplified without using a string literal.
-const player = state.entities["player1"]; +const player = state.entities.player1;🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts (1)
23-58: Good implementation of entity deep merging functionalityThe new
deepMergefunction properly handles recursive merging of nested objects, which fixes the issue with shallow merging that was previously happening. The function correctly handles different types of values (objects, primitives, arrays) and ensures that nested properties are preserved.However, there are several type safety concerns:
function deepMerge<T extends SchemaType>( target: EntityModels<T>, source: Partial<EntityModels<T>> ): EntityModels<T> { - const result = { ...target } as Record<string, any>; + const result = { ...target } as EntityModels<T>; for (const key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { if ( source[key] !== null && typeof source[key] === "object" && !Array.isArray(source[key]) ) { // If the property is an object in both source and target, recursively merge if ( key in target && typeof target[key] === "object" && !Array.isArray(target[key]) ) { result[key] = deepMerge(target[key], source[key]); } else { // If the key doesn't exist in target or isn't an object, just assign result[key] = source[key]; } } else { // For non-objects (primitives, arrays, null), just assign result[key] = source[key]; } } } return result; }🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sdk/src/__tests__/state.test.ts(1 hunks)packages/sdk/src/state/zustand.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/state/zustand.ts
[error] 30-30: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 101-121: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 108-108: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 119-119: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/__tests__/state.test.ts
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (3)
packages/sdk/src/__tests__/state.test.ts (1)
114-126: Good test case addition!The new test effectively validates that
mergeEntitiescorrectly preserves existing entity properties while updating only the specified ones. This covers the deep merge functionality being implemented.🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts (2)
101-121: Prefer for...of loops over forEach for better performanceThe forEach loop might lead to performance issues when working with large arrays. A for...of loop would be more efficient.
-Object.entries(entity.models).forEach( - ([namespace, namespaceModels]) => { +for (const [namespace, namespaceModels] of Object.entries(entity.models)) { const typedNamespace = namespace as keyof ParsedEntity<T>["models"]; if (!(typedNamespace in mergedModels)) { mergedModels[ typedNamespace as keyof typeof mergedModels - ] = {} as any; + ] = {} as EntityModels<T>; } // Use deep merge instead of Object.assign mergedModels[ typedNamespace as keyof typeof mergedModels ] = deepMerge( mergedModels[ typedNamespace as keyof typeof mergedModels ] as EntityModels<T>, namespaceModels - ) as any; + ) as EntityModels<T>; - } -); +}🧰 Tools
🪛 Biome (1.9.4)
[error] 101-121: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 108-108: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 119-119: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
88-129: Good refactoring of the mergeEntities methodThe improved implementation correctly handles deep merging of entity properties, ensuring that only specified properties are updated while preserving the rest. The new test confirms this functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-121: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 108-108: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 119-119: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/state/zustand.ts
Outdated
| const mergedModels: typeof existingEntity.models = | ||
| Object.assign({}, existingEntity.models); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider updating updateEntity method to use deepMerge as well
The mergeEntities method now uses deepMerge, but updateEntity still uses Object.assign at line 157. For consistency, consider updating the updateEntity method to use deepMerge as well.
🏁 Script executed:
#!/bin/bash
# Check if updateEntity is using Object.assign
rg -A 10 "updateEntity" packages/sdk/src/state/zustand.ts | rg "Object.assign"Length of output: 154
Action Required: Update updateEntity to use deepMerge for consistency
- In
packages/sdk/src/state/zustand.ts, theupdateEntitymethod (around line 157) still usesObject.assign({}, existingEntity.models), as confirmed by the shell script output. - Meanwhile, the
mergeEntitiesmethod already employsdeepMerge. For consistency in handling nested properties, please updateupdateEntityto also usedeepMergeinstead ofObject.assign.
MartianGreed
force-pushed
the
fix/zustand-merge-entities
branch
from
26d4dbf to
3ebce61
Compare
MartianGreed
force-pushed
the
fix/zustand-merge-entities
branch
from
3ebce61 to
e2a4ea5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/sdk/src/__tests__/state.test.ts (1)
123-123: Consider using object property access directly.Instead of accessing the entity using bracket notation, you can use dot notation when the property name is a known literal.
-const player = state.entities["player1"]; +const player = state.entities.player1;🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts (1)
150-169: Consider using for...of instead of forEach for better performance.When working with large arrays, using
for...ofcan be more performant thanforEach. This is especially important when dealing with entity collections that might grow large.- Object.entries(entity.models).forEach( - ([namespace, namespaceModels]) => { + for (const [namespace, namespaceModels] of Object.entries(entity.models)) { const typedNamespace = namespace as keyof ParsedEntity<T>["models"]; if (!(typedNamespace in mergedModels)) { mergedModels[ typedNamespace as keyof typeof mergedModels ] = {} as any; } mergedModels[ typedNamespace as keyof typeof mergedModels ] = deepMerge( mergedModels[ typedNamespace as keyof typeof mergedModels ] as MergedModels<T>, namespaceModels ) as any; - } - ); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 150-169: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 157-157: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 167-167: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/sharp-cooks-kiss.md(1 hunks)packages/sdk/src/__tests__/state.test.ts(1 hunks)packages/sdk/src/state/zustand.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/sharp-cooks-kiss.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/__tests__/state.test.ts
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts
[error] 30-30: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 119-119: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 150-169: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 157-157: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 167-167: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (4)
packages/sdk/src/__tests__/state.test.ts (1)
114-126: Great test for the new mergeEntities functionality!This test effectively verifies that
mergeEntitiescorrectly preserves existing properties while updating only the ones specified in the new entity. It shows that when merging entities, only the score is updated to 120 while the name property remains "Alice".🧰 Tools
🪛 Biome (1.9.4)
[error] 123-123: The computed expression can be simplified without the use of a string literal.
Unsafe fix: Use a literal key instead.
(lint/complexity/useLiteralKeys)
packages/sdk/src/state/zustand.ts (3)
23-58: Good implementation of the deep merge functionality!The
deepMergefunction correctly handles nested objects by recursively merging them while preserving other properties. This is a significant improvement over the previous shallow merging withObject.assign.However, there are a few areas for improvement:
- The function uses
Record<string, any>which bypasses type checking. Consider using more specific types.- The conditional logic is comprehensive but could be more concise.
A more type-safe implementation could look like:
function deepMerge<T extends SchemaType>( target: MergedModels<T>, source: Partial<MergedModels<T>> ): MergedModels<T> { - const result = { ...target } as Record<string, any>; + const result = { ...target } as MergedModels<T>; for (const key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { const sourceValue = source[key]; if ( - source[key] !== null && - typeof source[key] === "object" && - !Array.isArray(source[key]) + sourceValue !== null && + typeof sourceValue === "object" && + !Array.isArray(sourceValue) ) { // If the property is an object in both source and target, recursively merge if ( key in target && - typeof target[key] === "object" && - !Array.isArray(target[key]) + target[key] !== null && + typeof target[key] === "object" && + !Array.isArray(target[key]) ) { - result[key] = deepMerge(target[key], source[key]); + result[key] = deepMerge(target[key] as MergedModels<T>, sourceValue as Partial<MergedModels<T>>); } else { // If the key doesn't exist in target or isn't an object, just assign - result[key] = source[key]; + result[key] = sourceValue; } } else { // For non-objects (primitives, arrays, null), just assign - result[key] = source[key]; + result[key] = sourceValue; } } } return result; }🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
88-126: Good refactoring of mergeEntities to use deepMerge!The implementation correctly checks for existing entities and properly merges the models using the new
deepMergefunction, which will handle nested properties better than the previousObject.assign.🧰 Tools
🪛 Biome (1.9.4)
[error] 101-121: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 108-108: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 119-119: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
133-176: Well done updating updateEntity to use deepMerge!Excellent work addressing the previous review feedback by updating the
updateEntitymethod to usedeepMergeas well. This ensures consistency betweenmergeEntitiesandupdateEntitymethods and improves how nested properties are handled.🧰 Tools
🪛 Biome (1.9.4)
[error] 150-169: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 157-157: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 167-167: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Tests
Documentation