diff --git a/src/display/mixins/Childrenable.js b/src/display/mixins/Childrenable.js index 62f516b6..d32be3e6 100644 --- a/src/display/mixins/Childrenable.js +++ b/src/display/mixins/Childrenable.js @@ -1,49 +1,23 @@ -import { isValidationError } from 'zod-validation-error'; import { deepMerge } from '../../utils/deepmerge/deepmerge'; import { findIndexByPriority } from '../../utils/findIndexByPriority'; -import { validate } from '../../utils/validator'; import { mapDataSchema } from '../data-schema/element-schema'; import { newElement } from '../elements/creator'; import { UPDATE_STAGES } from './constants'; +import { validateAndPrepareChanges } from './utils'; const KEYS = ['children']; export const Childrenable = (superClass) => { const MixedClass = class extends superClass { _applyChildren(relevantChanges, options) { - const { children: childrenChanges } = relevantChanges; + let { children: childrenChanges } = relevantChanges; let elements = [...this.children]; - // --- Start of Performance Optimization --- - // This logic mirrors the optimization in `Componentsable.js`. - // Instead of validating each new element inside the loop, we identify - // new elements beforehand and validate them all at once. - - // 1. Filter out only the definitions for new elements. - const newElementDefs = []; - const newElementIndices = []; // Store original indices to update the array later. - childrenChanges.forEach((change, index) => { - if (findIndexByPriority(elements, change) === -1) { - newElementDefs.push(change); - newElementIndices.push(index); - } - }); - - // 2. If new elements exist, perform a single batch validation. - // This is far more efficient than validating one-by-one inside the loop. - if (newElementDefs.length > 0) { - const validatedNewDefs = validate(newElementDefs, mapDataSchema); - if (isValidationError(validatedNewDefs)) { - throw validatedNewDefs; - } - - // 3. Update the original changes array with the validated, default-filled definitions. - validatedNewDefs.forEach((validatedDef, i) => { - const originalIndex = newElementIndices[i]; - childrenChanges[originalIndex] = validatedDef; - }); - } - // --- End of Performance Optimization --- + childrenChanges = validateAndPrepareChanges( + elements, + childrenChanges, + mapDataSchema, + ); if (options.arrayMerge === 'replace') { elements.forEach((element) => { diff --git a/src/display/mixins/Componentsable.js b/src/display/mixins/Componentsable.js index f54eaf2e..e92f6dad 100644 --- a/src/display/mixins/Componentsable.js +++ b/src/display/mixins/Componentsable.js @@ -1,52 +1,23 @@ -import { isValidationError } from 'zod-validation-error'; import { deepMerge } from '../../utils/deepmerge/deepmerge'; import { findIndexByPriority } from '../../utils/findIndexByPriority'; -import { validate } from '../../utils/validator'; import { newComponent } from '../components/creator'; import { componentArraySchema } from '../data-schema/component-schema'; import { UPDATE_STAGES } from './constants'; +import { validateAndPrepareChanges } from './utils'; const KEYS = ['components']; export const Componentsable = (superClass) => { const MixedClass = class extends superClass { _applyComponents(relevantChanges, options) { - const { components: componentsChanges } = relevantChanges; + let { components: componentsChanges } = relevantChanges; let components = [...this.children]; - // --- Start of Performance Optimization --- - // The main performance bottleneck is calling `validate` for each new component - // inside a loop. To solve this, we pre-filter new components and - // validate them all at once. - - // 1. Filter out only the definitions for components that need to be newly created. - const newComponentDefs = []; - const newComponentIndices = []; // Store original indices to update the array later. - componentsChanges.forEach((change, index) => { - if (findIndexByPriority(components, change) === -1) { - newComponentDefs.push(change); - newComponentIndices.push(index); - } - }); - - // 2. If there are new components, perform a single batch validation. - // This is far more efficient than validating one-by-one inside the loop. - if (newComponentDefs.length > 0) { - const validatedNewDefs = validate( - newComponentDefs, - componentArraySchema, - ); - if (isValidationError(validatedNewDefs)) { - throw validatedNewDefs; - } - - // 3. Update the original changes array with the validated, default-filled definitions. - validatedNewDefs.forEach((validatedDef, i) => { - const originalIndex = newComponentIndices[i]; - componentsChanges[originalIndex] = validatedDef; - }); - } - // --- End of Performance Optimization --- + componentsChanges = validateAndPrepareChanges( + components, + componentsChanges, + componentArraySchema, + ); if (options.arrayMerge === 'replace') { components.forEach((component) => { diff --git a/src/display/mixins/utils.js b/src/display/mixins/utils.js index 733e1597..df6ab2aa 100644 --- a/src/display/mixins/utils.js +++ b/src/display/mixins/utils.js @@ -1,4 +1,7 @@ import gsap from 'gsap'; +import { isValidationError } from 'zod-validation-error'; +import { findIndexByPriority } from '../../utils/findIndexByPriority'; +import { validate } from '../../utils/validator'; export const tweensOf = (object) => gsap.getTweensOf(object); @@ -60,3 +63,46 @@ export const calcSize = (component, { source, size }) => { export const mixins = (baseClass, ...mixins) => { return mixins.reduce((target, mixin) => mixin(target), baseClass); }; + +/** + * Utility function to validate and prepare a new array of changes. + * This function filters out only the new elements (not present in currentElements), + * and performs batch validation using a Zod schema. + * + * @param {Array} currentElements - Array of current child elements (components) in the DOM + * @param {Array} changes - Array of change data to apply + * @param {import('zod').ZodSchema} schema - Zod schema to use for validation + * @returns {Array} The changes array, with validated and default-filled data + */ +export const validateAndPrepareChanges = (currentElements, changes, schema) => { + const preparedChanges = [...changes]; + const used = new Set(); + const newElementDefs = []; + const newElementIndices = []; + + preparedChanges.forEach((change, index) => { + const foundIndex = findIndexByPriority(currentElements, change, used); + if (foundIndex === -1) { + newElementDefs.push(change); + newElementIndices.push(index); + } else { + used.add(foundIndex); + } + }); + + // Perform batch validation only if there are new elements to be added + if (newElementDefs.length > 0) { + const validatedNewDefs = validate(newElementDefs, schema); + if (isValidationError(validatedNewDefs)) { + throw validatedNewDefs; + } + + // Update the original changes array with the validated definitions (including defaults) + validatedNewDefs.forEach((validatedDef, i) => { + const originalIndex = newElementIndices[i]; + preparedChanges[originalIndex] = validatedDef; + }); + } + + return preparedChanges; +}; diff --git a/src/tests/render/patchmap.test.js b/src/tests/render/patchmap.test.js index 354883f5..eccdcecf 100644 --- a/src/tests/render/patchmap.test.js +++ b/src/tests/render/patchmap.test.js @@ -45,6 +45,9 @@ const sampleData = [ radius: 6, }, }, + { type: 'text', text: 'text-1' }, + { type: 'text', text: 'text-2' }, + { type: 'text', id: 'new-text' }, ], attrs: { x: 200, y: 300 }, }, @@ -89,6 +92,12 @@ describe('patchmap test', () => { expect(item).toBeDefined(); expect(item.id).toBe('item-1'); + const itemChildren = [...item.children]; + expect(itemChildren.length).toBe(4); + expect(itemChildren[0].type).toBe('background'); + expect(itemChildren[1].type).toBe('text'); + expect(itemChildren[2].type).toBe('text'); + const gridItems = grid.children; expect(gridItems.length).toBe(5); }); @@ -183,6 +192,30 @@ describe('patchmap test', () => { expect(group.x).toBe(350); expect(group.y).toBe(250); }); + + it('should handle array updates with duplicate ids correctly', () => { + patchmap.update({ + path: '$..[?(@.id=="item-1")]', + changes: { + components: [ + { type: 'text', id: 'new-text', text: '2' }, + { type: 'text', id: 'B', text: '99' }, + { type: 'text', id: 'new-text', text: '3' }, + ], + }, + }); + + const item = patchmap.selector('$..[?(@.id=="item-1")]')[0]; + expect(item.children.length).toBe(6); + + const newTextChildren = item.children.filter((c) => c.id === 'new-text'); + expect(newTextChildren.length).toBe(2); + expect(newTextChildren.map((c) => c.text).sort()).toEqual(['2', '3']); + + const childB = item.children.find((c) => c.id === 'B'); + expect(childB).toBeDefined(); + expect(childB.text).toBe('99'); + }); }); describe('select', () => { diff --git a/src/utils/deepmerge/deepmerge.js b/src/utils/deepmerge/deepmerge.js index 908304b6..ee3835a4 100644 --- a/src/utils/deepmerge/deepmerge.js +++ b/src/utils/deepmerge/deepmerge.js @@ -75,9 +75,11 @@ const mergeArray = (target, source, options, visited) => { } } else if (i < merged.length) { merged[i] = item; + used.add(i); return; } merged.push(item); + used.add(merged.length - 1); }); return merged; diff --git a/src/utils/deepmerge/deepmerge.test.js b/src/utils/deepmerge/deepmerge.test.js index 0ce099ba..88062216 100644 --- a/src/utils/deepmerge/deepmerge.test.js +++ b/src/utils/deepmerge/deepmerge.test.js @@ -197,6 +197,30 @@ describe('deepMerge – arrayMerge by id → label → type', () => { ], }, ], + [ + { + components: [], + }, + { + components: [ + { type: 'text', text: '5' }, + { type: 'text', text: '6' }, + { type: 'text', text: '7' }, + ], + }, + { + components: [ + { type: 'text', text: '5' }, + { type: 'text', text: '6' }, + { type: 'text', text: '7' }, + ], + }, + ], + [ + { components: [{ type: 'text', text: 'original' }] }, + { components: [null, { type: 'text', text: 'new' }] }, + { components: [null, { type: 'text', text: 'new' }] }, + ], ])('Case %#', (left, right, expected) => { expect(deepMerge(left, right)).toEqual(expected); }); diff --git a/src/utils/findIndexByPriority.js b/src/utils/findIndexByPriority.js index 02525e86..2df459b1 100644 --- a/src/utils/findIndexByPriority.js +++ b/src/utils/findIndexByPriority.js @@ -27,7 +27,7 @@ export const findIndexByPriority = ( for (const key of priorityKeys) { if (key in criteria) { return arr.findIndex( - (item, idx) => !usedIndexes.has(idx) && item[key] === criteria[key], + (item, idx) => !usedIndexes.has(idx) && item?.[key] === criteria[key], ); } }