Skip to content

Conversation

@dayongkr
Copy link
Collaborator

PR Description

Problem

es-toolkit/compat's merge() produces different results than Lodash when a nested object property is replaced by an array from the source.

import _ from 'lodash';
import { merge } from 'es-toolkit/compat';

// Lodash
_.merge({ x: { a: 2 } }, { x: ['1'] }).x    // => ['1']

// es-toolkit/compat (before fix)
merge({ x: { a: 2 } }, { x: ['1'] }).x      // => ['1', a: 2] ❌

Root Cause

In mergeWith.ts, when source is an array, the code treated all objects the same - copying every property to the new array:

// Before (Bug)
if (Array.isArray(sourceValue)) {
  if (typeof targetValue === 'object' && targetValue != null) {
    // Copies ALL keys from any object to array ❌
    const targetKeys = Reflect.ownKeys(targetValue);
    for (...) { cloned[targetKey] = targetValue[targetKey]; }
  }
}

Lodash (_baseMergeDeep.js lines 53-70) distinguishes three cases
using isArrayLikeObject:

// Lodash _baseMergeDeep.js
if (isArr || isBuff || isTyped) {
  if (isArray(objValue)) {
    newValue = objValue;              // Array: keep as-is
  } else if (isArrayLikeObject(objValue)) {
    newValue = copyArray(objValue);   // ArrayLikeObject: copy 
numeric indices only
  } else if (isBuff) {
    isCommon = false;
    newValue = cloneBuffer(srcValue, true);
  } else if (isTyped) {
    isCommon = false;
    newValue = cloneTypedArray(srcValue, true);
  } else {
    newValue = [];                    // Plain object: empty array
  }
}

Solution

Use isArrayLikeObject to match Lodash's logic:

if (Array.isArray(sourceValue)) {
  if (Array.isArray(targetValue)) {
    // Array: copy all keys (preserves custom properties)
    const cloned: any = [];
    const targetKeys = Reflect.ownKeys(targetValue);
    for (...) { cloned[targetKey] = targetValue[targetKey]; }
    targetValue = cloned;
  } else if (isArrayLikeObject(targetValue)) {
    // ArrayLikeObject: copy numeric indices only
    const cloned: any = [];
    for (let i = 0; i < targetValue.length; i++) {
      cloned[i] = targetValue[i];
    }
    targetValue = cloned;
  } else {
    // Plain object: empty array
    targetValue = [];
  }
}

Test

Added test cases in merge.spec.ts and mergeWith.spec.ts:

it('should not preserve object properties when nested object is 
replaced by array', () => {
  const actual = merge({ x: { a: 2 } }, { x: ['1'] });

  expect(actual.x).toEqual(['1']);
  expect(Object.keys(actual.x)).toEqual(['0']);
  expect((actual.x as any).a).toBeUndefined();
});

@dayongkr dayongkr requested a review from raon0211 as a code owner November 26, 2025 15:27
Copilot AI review requested due to automatic review settings November 26, 2025 15:27
@vercel
Copy link

vercel bot commented Nov 26, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
es-toolkit Ready Ready Preview Comment Nov 26, 2025 3:30pm

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.97%. Comparing base (335011b) to head (96d3251).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1542   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files         474      474           
  Lines        4492     4498    +6     
  Branches     1313     1314    +1     
=======================================
+ Hits         4491     4497    +6     
  Misses          1        1           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot finished reviewing on behalf of dayongkr November 26, 2025 15:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a compatibility issue in merge() and mergeWith() where plain object properties were incorrectly preserved when the object was replaced by an array from the source, diverging from Lodash's behavior.

Key Changes:

  • Modified mergeWithDeep() to use isArrayLikeObject to distinguish between arrays, array-like objects, and plain objects when the source value is an array
  • Added test cases to verify plain object properties are not preserved when replaced by arrays

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/compat/object/mergeWith.ts Implements three-case logic for handling arrays, array-like objects, and plain objects when merging with array sources, using isArrayLikeObject to match Lodash behavior
src/compat/object/mergeWith.spec.ts Adds test case verifying plain object properties don't persist when replaced by an array
src/compat/object/merge.spec.ts Adds test case verifying plain object properties don't persist when replaced by an array

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compat/merge behavior differs from Lodash when merging array into object

3 participants