feat #19
feat #19
Conversation
WalkthroughThis pull request adds a new deep merge functionality through the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DM as $deepMerge
participant ML as Merge Logic
U->>DM: Call $deepMerge(target, source)
DM->>DM: Validate target and source objects
DM->>ML: Recursively merge properties
ML-->>DM: Return merged property values
DM->>U: Return final merged object
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 (
|
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
- Coverage 95.17% 94.82% -0.36%
==========================================
Files 3 3
Lines 311 367 +56
Branches 89 105 +16
==========================================
+ Hits 296 348 +52
- Misses 15 19 +4 ☔ View full report in Codecov by Sentry. |
![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 (8)
object.test.ts (1)
177-177: Consider adding English comments for better accessibility.The Japanese comments describing test cases could be supplemented with English translations for broader accessibility to international contributors.
- // 基本的なオブジェクトのマージ + // Basic object merging (基本的なオブジェクトのマージ)Also applies to: 183-183, 189-189, 195-195, 204-204
README.md (1)
80-95: Consider adding clarification about array merging behavior.The example shows that arrays are concatenated during merging, but this might not be immediately obvious to all users. Consider adding a brief explanation to clarify this specific behavior.
/* Output: { a: 1, b: { c: "hello", d: [1, 2, 3, 4], // Arrays are concatenated, not replaced e: true }, f: "world" } */type.test.ts (1)
145-145: Consider adding English comments for better accessibility.As with the object tests, consider supplementing the Japanese comments with English translations to improve accessibility for international contributors.
- // 基本的なオブジェクトのマージ + // Basic object merging (基本的なオブジェクトのマージ)Also applies to: 151-151, 161-161, 171-171, 177-177
object.ts (5)
79-85: Replace Japanese comment with English for consistency.The validation code is correct, but there's a Japanese comment that should be translated to English to maintain consistency with the rest of the codebase.
- // 基本的な検証 + // Basic validation
87-91: Replace Japanese comment with English for consistency.The merge function is well-defined with appropriate generic types, but there's another Japanese comment that should be translated to English.
- // 2つのオブジェクトをマージする内部関数 + // Internal function to merge two objects
96-129: Consider extracting the object check condition into a helper function.The deep merging logic is sound, but the condition to check if values are objects (lines 103-110) is lengthy and repeated elsewhere. Consider extracting it into a helper function for better readability and maintainability.
+ // Helper function to check if a value is a non-null, non-undefined object + const isObject = (value: unknown): boolean => { + return value !== null && + value !== undefined && + typeof value === "object"; + }; // In the merge function: if ( - targetValue !== null && - targetValue !== undefined && - sourceValue !== null && - sourceValue !== undefined && - typeof targetValue === "object" && - typeof sourceValue === "object" + isObject(targetValue) && isObject(sourceValue) ) { // ... rest of the logic
75-135: The function doesn't support merging more than two objects.The JSDoc mentions "Deeply merges two or more objects" but the implementation only accepts two objects. Consider extending the function to handle multiple objects using rest parameters.
-export const $deepMerge = <T extends object, U extends object>( - target: T, - source: U, -): DeepMerge<T, U> => { +export const $deepMerge = <T extends object, U extends object, V extends object[]>( + target: T, + source: U, + ...sources: V +): DeepMerge<T, U> & DeepMerge<DeepMerge<T, U>, V[number]> => { // Basic validation if (typeof target !== "object" || target === null) { throw new Error("expected a non-null object for the first argument"); } if (typeof source !== "object" || source === null) { throw new Error("expected a non-null object for the second argument"); } + + // Validate additional sources + for (let i = 0; i < sources.length; i++) { + if (typeof sources[i] !== "object" || sources[i] === null) { + throw new Error(`expected a non-null object for argument ${i + 3}`); + } + } // Internal function to merge two objects const merge = <A extends object, B extends object>( target: A, source: B, ): DeepMerge<A, B> => { // Implementation remains the same }; - return merge(target, source); + // Merge the first two objects + let result = merge(target, source); + + // Merge any additional objects + for (const src of sources) { + result = merge(result, src); + } + + return result as DeepMerge<T, U> & DeepMerge<DeepMerge<T, U>, V[number]>;
75-135: Add protection against circular references.The recursive merging approach could potentially cause stack overflow errors if there are circular references in the objects being merged.
Consider adding a simple mechanism to detect circular references, such as using a WeakMap to track already processed objects:
export const $deepMerge = <T extends object, U extends object>( target: T, source: U, ): DeepMerge<T, U> => { // Basic validation if (typeof target !== "object" || target === null) { throw new Error("expected a non-null object for the first argument"); } if (typeof source !== "object" || source === null) { throw new Error("expected a non-null object for the second argument"); } + // Track processed objects to avoid circular references + const processedObjects = new WeakMap(); + // Internal function to merge two objects const merge = <A extends object, B extends object>( target: A, source: B, + path = "" ): DeepMerge<A, B> => { + // Check for circular references + if (processedObjects.has(target) && processedObjects.get(target).has(source)) { + return processedObjects.get(target).get(source); + } + + // Mark these objects as being processed + if (!processedObjects.has(target)) { + processedObjects.set(target, new WeakMap()); + } + if (!processedObjects.get(target).has(source)) { + const result = { ...target } as Record<PropertyKey, unknown>; + processedObjects.get(target).set(source, result); + + // Rest of the merge logic + // ... + + return result as DeepMerge<A, B>; + } + // Result object creation and merging logic // ... }; return merge(target, source); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(2 hunks)deno.json(1 hunks)object.test.ts(2 hunks)object.ts(1 hunks)type.test.ts(2 hunks)types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deno.json
🔇 Additional comments (18)
object.test.ts (3)
3-3: Good addition of the$deepMergeimport.The import statement has been properly updated to include the new
$deepMergefunction alongside existing imports.
176-209: Well-structured test cases for$deepMerge.The test cases comprehensively cover various scenarios:
- Basic object merging
- Nested object merging
- Array merging
- Complex nested structures
- Arrays within nested objects
This ensures the functionality works correctly across different use cases.
211-240: Thorough error handling tests for$deepMerge.The tests properly verify that the function rejects non-object arguments with appropriate error messages. This ensures robust input validation.
types.ts (2)
220-255: Well-implementedDeepMergetype with comprehensive JSDoc.The type implementation is robust and handles various edge cases:
- Recursive merging of nested objects
- Special handling for arrays
- Property override behavior
The JSDoc comments are excellent, providing clear explanation of purpose, behavior, and examples.
242-243:❓ Verification inconclusive
Verify array merging behavior is consistent with runtime implementation.
The type system creates a union of array element types (
Array<T1 | U1>), while the runtime implementation likely concatenates arrays. This could lead to confusion for users who expect the type to reflect the concatenated structure.
🏁 Script executed:
#!/bin/bash # Description: Verify that the runtime implementation of $deepMerge for arrays is consistent with type definition. # Find the implementation of $deepMerge in object.ts to see how arrays are handled rg -A 20 -B 5 "function \$deepMerge"Length of output: 37
Action Required: Manually Verify the Runtime Array Merging Behavior for $deepMerge
The automated search for the implementation in
object.tsdid not return any output, so we couldn’t automatically confirm how arrays are handled at runtime. The type definition merges array element types into a union (Array<T1 | U1>), but it remains unclear whether the runtime function concatenates arrays or behaves differently.
- Next Steps:
- Manually inspect the implementation of
$deepMerge(or its equivalent) to determine if array merging is done via concatenation.- Confirm that the type behavior (producing a union of element types) accurately reflects the actual runtime behavior.
- If there is a divergence between the type definition and runtime behavior (e.g., if runtime concatenates arrays), consider updating the type annotations or documentation to prevent any user confusion.
README.md (2)
63-95: Clear and informative examples for$deepMergefunction.The examples clearly demonstrate how to use the
$deepMergefunction with:
- Basic object merging
- Nested property handling
- Array merging behavior
The output comments provide helpful visualization of the expected results.
405-436: Good demonstration of theDeepMergetype utility.The type examples effectively show how the
DeepMergetype works with complex object structures and array types. The expected result is well-documented.type.test.ts (3)
3-3: Good addition ofDeepMergeto the imports.The import statement has been properly updated to include the new
DeepMergetype utility.
144-189: Comprehensive type tests forDeepMerge.The type tests thoroughly verify that
DeepMergeworks correctly across different scenarios:
- Basic object merging
- Nested object merging
- Array type merging
- Primitive value overrides
- Complex nested structures
Using
AssertTrue<IsExact<...>>ensures strict type checking.
162-169:✅ Verification successful
Verify type behavior with runtime array merging.
The test correctly expects array types to be merged as unions (
Array<string | boolean>), but ensure this is consistent with how the runtime implementation handles arrays (likely through concatenation).
🏁 Script executed:
#!/bin/bash # Description: Compare the array type behavior with runtime implementation. # Look for test cases that specifically test array merging behavior rg -A 10 "array" object.test.tsLength of output: 2314
Array Merging Behavior Verified
The type tests correctly merge array types into unions (e.g.
Array<string | boolean>), and the runtime tests for$deepMergeinobject.test.tsconfirm that arrays are merged via concatenation (e.g.[1,2,4,5]). This alignment between type-level expectations and runtime behavior is as intended. Please continue to ensure that any future changes to deep merging preserve this consistency.object.ts (8)
63-63: Import is correctly positioned.The import for the
DeepMergetype is appropriately placed and will be used to define the return type of the new function.
65-74: JSDoc is well structured and informative.The documentation clearly describes the function purpose, parameters, return type, and potential exceptions. This maintains consistency with the documentation style of other functions in the file.
75-78: Function signature correctly uses generics.The generic type parameters
TandUappropriately constrain both parameters to be objects while allowing flexibility in the specific object types that can be merged.
92-94: The spread operator creates a shallow copy of the target.This is correct for initializing the result object. Creating a new object ensures that the original objects aren't modified.
111-127: The array merging strategy should be documented.The function concatenates arrays rather than recursively merging their elements. This is a valid approach but should be explicitly mentioned in the JSDoc since users might expect different behavior.
The array handling logic looks correct, but consider adding a note to the JSDoc about how arrays are handled:
* @returns {DeepMerge<T, U>} A new object with all properties deeply merged. +* @remarks Arrays are concatenated rather than deeply merged. * @throws Will throw an error if any argument is not a non-null object.
103-110: Check object type after non-null and non-undefined check.The current implementation checks
typeof value === "object"after confirming the value is not null or undefined, which is good practice. This is the correct order becausetypeof nullreturns "object" in JavaScript.
131-132: Type casting is used appropriately.The result is correctly cast to
DeepMerge<A, B>which should ensure proper type information is preserved.
134-135: Function execution is straightforward.The implementation correctly calls the inner merge function with the target and source objects.
Summary by CodeRabbit
New Features
Documentation
Chores