#3472089: Improve/Refactor replaceUUIDsAndUpdateModel() and insertMultipleNodes()
Closes #3472089
Merge request reports
Activity
added 1 commit
1 1 // cspell:ignore idontexist 2 import { isChildNode } from '../../../../../ui/src/features/layout/layoutUtils'; 2 import { 3 isChildNode, 4 replaceUUIDsAndUpdateModel, 5 insertMultipleNodesUtil, 6 } from '../../../../../ui/src/features/layout/layoutUtils'; 7 import _ from 'lodash'; 3 8 4 9 let layout; 5 before('Load fixture', function () { 10 before('Load fixtures', function () { 11 // Load layout fixture changed this line in version 4 of the diff
126 }, 127 ], 128 }; 129 130 const to = [1]; // Inserting at index 1 131 const model = {}; // Your initial model, if needed 132 133 // Call the utility function 134 const { newLayout, updatedModel } = insertMultipleNodesUtil( 135 layout, 136 newNodes, 137 to, 138 model, 139 ); 140 141 // Assertions changed this line in version 4 of the diff
115 }; 116 117 const newNodes = { 118 children: [ 119 { 120 nodeType: 'component', 121 type: 'newComponent1', 122 }, 123 { 124 nodeType: 'component', 125 type: 'newComponent2', 126 }, 127 ], 128 }; 129 130 const to = [1]; // Inserting at index 1 changed this line in version 4 of the diff
added 5 commits
-
57985bb6...be472031 - 4 commits from branch
project:0.x
- 8e23766e - Merge branch experience_builder:0.x into 3472089-refactor-replaceUUIDsAndUpdateModel
-
57985bb6...be472031 - 4 commits from branch
74 } 75 76 /** 77 * Replace UUIDs in a layout node and its corresponding model. 78 * @param node - The layout node to update. 79 * @param model - The corresponding model to update. 80 * @returns An updated model and a updated state. 81 */ 82 export function replaceUUIDsAndUpdateModel( 83 node: LayoutNode, 84 model: ComponentModels, 85 ): { 86 updatedNode: LayoutNode; 87 updatedModel: ComponentModels; 88 } { 89 var oldToNewUUIDMap: Record<string, string> = {}; changed this line in version 6 of the diff
- Resolved by Jesse Baker
We should not be using
var
anywhere so I'm not sure why this was changed from let/const to var all over.I think moving these functions out to the utils file and adding tests is a good step forward but I think the intention was to try and reduce the complexity and improve the readability of these functions more than just moving them to another place and making minor changes.
38 46 } 39 47 } 40 48 49 export function insertMultipleNodesUtil( 50 layout: LayoutNode, 51 newNodes: LayoutNode, 52 to: number[], 53 model: ComponentModels, 54 ): { newLayout: LayoutNode; updatedModel: ComponentModels } { 55 let updatedModel: ComponentModels = { ...model }; 56 const nodesToInsert: LayoutNode[] = _.cloneDeep(newNodes.children); 57 let newLayout: LayoutNode = _.cloneDeep(layout); 58 59 for (let i = nodesToInsert.length - 1; i >= 0; i--) { 69 70 /** 71 * Replace UUIDs in a layout node and its corresponding model. 72 * @param node - The layout node to update. 73 * @param model - The corresponding model to update. 74 * @returns An updated model and a updated state. 75 */ 76 export function replaceUUIDsAndUpdateModel( 77 node: LayoutNode, 78 model: ComponentModels, 79 ): { 80 updatedNode: LayoutNode; 81 updatedModel: ComponentModels; 82 } { 83 let oldToNewUUIDMap: Record<string, string> = {}; 84 let updatedModel: ComponentModels = {}; - Comment on lines +83 to +84
72 * @param node - The layout node to update. 73 * @param model - The corresponding model to update. 74 * @returns An updated model and a updated state. 75 */ 76 export function replaceUUIDsAndUpdateModel( 77 node: LayoutNode, 78 model: ComponentModels, 79 ): { 80 updatedNode: LayoutNode; 81 updatedModel: ComponentModels; 82 } { 83 let oldToNewUUIDMap: Record<string, string> = {}; 84 let updatedModel: ComponentModels = {}; 85 86 function replaceUUIDs(node: LayoutNode): LayoutNode { 87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() }); 84 let updatedModel: ComponentModels = {}; 85 86 function replaceUUIDs(node: LayoutNode): LayoutNode { 87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() }); 88 89 oldToNewUUIDMap[node.uuid] = newNode.uuid; 90 91 if (newNode.children) { 92 newNode.children = newNode.children.map(replaceUUIDs); 93 } 94 return newNode; 95 } 96 97 let updatedNode = replaceUUIDs(node); 98 99 for (let oldUUID in model) { 86 function replaceUUIDs(node: LayoutNode): LayoutNode { 87 let newNode: LayoutNode = Object.assign({}, node, { uuid: uuidv4() }); 88 89 oldToNewUUIDMap[node.uuid] = newNode.uuid; 90 91 if (newNode.children) { 92 newNode.children = newNode.children.map(replaceUUIDs); 93 } 94 return newNode; 95 } 96 97 let updatedNode = replaceUUIDs(node); 98 99 for (let oldUUID in model) { 100 if (Object.prototype.hasOwnProperty.call(model, oldUUID)) { 101 let newUUID = oldToNewUUIDMap[oldUUID]; Fantastic work on adding the tests, that's really helpful.
I do think that moving these function into the layoutUtils.ts is a good start but the actual functions themselves are no different than they were before.
In moving them, a number of consts were changed to let which I think is a regression so they should be changed back. Also some of the comments didn't make it across which I also think is step backwards.
Now, with tests in place it would be nice to see if these can be improved for better readability and/or performance.