refactor(utils): improve performance of copyEmptyArrayProps function (#1816)

* test(utils): add test for performance with large arrays

* fix(utils): improve performance of copyEmptyArrayProps fn

* fix(test): validate test with 200 ms

* docs(release): add changeset

Author: qmateub

.changeset/wise-timers-shake.md

@@ -0,0 +1,5 @@
+---
+'@commercetools/sync-actions': minor
+---
+
+feat(sync-actions): improve performance for large arrays comparisons"

packages/sync-actions/src/utils/copy-empty-array-props.js

@@ -13,46 +13,41 @@ const CUSTOM = 'custom'
 export default function copyEmptyArrayProps(oldObj = {}, newObj = {}) {
   if (!isNil(oldObj) && !isNil(newObj)) {
     const nextObjectWithEmptyArray = Object.entries(oldObj).reduce(
-      (nextObject, [key, value]) => {
-        const merged = {
-          ...newObj,
-          ...nextObject,
-        }
-
+      (merged, [key, value]) => {
         // Ignore CUSTOM key as this object is dynamic and its up to the user to dynamically change it
         // todo, it would be better if we pass it as ignored keys param
         if (key === CUSTOM) return merged
 
         if (Array.isArray(value) && newObj[key] && newObj[key].length >= 1) {
           /* eslint-disable no-plusplus */
+          const hashMapValue = value.reduce((acc, val) => {
+            acc[val.id] = val
+            return acc
+          }, {})
           for (let i = 0; i < newObj[key].length; i++) {
             if (
               !isNil(newObj[key][i]) &&
               typeof newObj[key][i] === 'object' &&
               !isNil(newObj[key][i].id)
             ) {
               // Since its unordered array elements then check if the element on `oldObj` exists by id
-              const foundObject = value.find(
-                (v) => Number(v.id) === Number(newObj[key][i].id)
-              )
+              const foundObject = hashMapValue[newObj[key][i].id]
               if (!isNil(foundObject)) {
                 const [, nestedObject] = copyEmptyArrayProps(
                   foundObject,
                   newObj[key][i]
                 )
                 /* eslint-disable no-param-reassign */
-                newObj[key][i] = nestedObject
+                merged[key][i] = nestedObject
               }
             }
           }
 
           return merged
         }
         if (Array.isArray(value)) {
-          return {
-            ...merged,
-            [key]: isNil(newObj[key]) ? [] : newObj[key],
-          }
+          merged[key] = isNil(newObj[key]) ? [] : newObj[key]
+          return merged
         }
         if (
           !isNil(newObj[key]) &&
@@ -62,10 +57,8 @@ export default function copyEmptyArrayProps(oldObj = {}, newObj = {}) {
           !(value instanceof Date)
         ) {
           const [, nestedObject] = copyEmptyArrayProps(value, newObj[key])
-          return {
-            ...merged,
-            [key]: nestedObject,
-          }
+          merged[key] = nestedObject
+          return merged
         }
         return merged
       },

packages/sync-actions/test/utils/copy-empty-array-props.spec.js

@@ -1,3 +1,4 @@
+import { performance } from 'perf_hooks'
 import copyEmptyArrayProps from '../../src/utils/copy-empty-array-props'
 
 describe('null check on root value', () => {
@@ -306,3 +307,25 @@ test('shouldnt change objects since there is no arrays to copy', () => {
   expect(old).toEqual(oldObj)
   expect(fixedNewObj).toEqual(newObj)
 })
+
+test('performance test for large arrays should be less than 200 ms', () => {
+  const oldObj = {
+    addresses: Array(5000)
+      .fill(null)
+      .map((a, index) => ({ id: `address-${index}` })),
+  }
+
+  const newObj = {
+    addresses: Array(5000)
+      .fill(null)
+      .map((a, index) => ({ id: `address-${index}` })),
+  }
+
+  const start = performance.now()
+  const [old, fixedNewObj] = copyEmptyArrayProps(oldObj, newObj)
+  const end = performance.now()
+
+  expect(old).toEqual(oldObj)
+  expect(fixedNewObj).toEqual(newObj)
+  expect(end - start).toBeLessThan(200)
+})