From 112d6b883d31ee2aba1cff396b580bbcce4a22a4 Mon Sep 17 00:00:00 2001 From: Alex Grozav Date: Thu, 25 Jul 2024 15:26:50 +0300 Subject: [PATCH] feat(editor): Add several performance improvements when adding nodes in new canvas (no-changelog) (#10170) --- .../src/components/canvas/WorkflowCanvas.vue | 2 + .../__tests__/useCanvasOperations.spec.ts | 143 +++++++++++------- .../src/composables/useCanvasOperations.ts | 118 +++++++++------ .../src/composables/useWorkflowHelpers.ts | 2 +- packages/editor-ui/src/views/NodeView.v2.vue | 6 +- packages/editor-ui/src/views/NodeView.vue | 2 +- .../src/views/WorkflowExecutionsView.vue | 2 +- 7 files changed, 170 insertions(+), 105 deletions(-) diff --git a/packages/editor-ui/src/components/canvas/WorkflowCanvas.vue b/packages/editor-ui/src/components/canvas/WorkflowCanvas.vue index 782568692e..19482bf699 100644 --- a/packages/editor-ui/src/components/canvas/WorkflowCanvas.vue +++ b/packages/editor-ui/src/components/canvas/WorkflowCanvas.vue @@ -5,6 +5,7 @@ import type { Workflow } from 'n8n-workflow'; import type { IWorkflowDb } from '@/Interface'; import { useCanvasMapping } from '@/composables/useCanvasMapping'; import type { EventBus } from 'n8n-design-system'; +import { createEventBus } from 'n8n-design-system'; defineOptions({ inheritAttrs: false, @@ -21,6 +22,7 @@ const props = withDefaults( }>(), { id: 'canvas', + eventBus: () => createEventBus(), fallbackNodes: () => [], }, ); diff --git a/packages/editor-ui/src/composables/__tests__/useCanvasOperations.spec.ts b/packages/editor-ui/src/composables/__tests__/useCanvasOperations.spec.ts index 181cd694ad..ffdbe8be52 100644 --- a/packages/editor-ui/src/composables/__tests__/useCanvasOperations.spec.ts +++ b/packages/editor-ui/src/composables/__tests__/useCanvasOperations.spec.ts @@ -24,6 +24,7 @@ import { useCredentialsStore } from '@/stores/credentials.store'; import { useWorkflowHelpers } from '@/composables/useWorkflowHelpers'; import { telemetry } from '@/plugins/telemetry'; import { useClipboard } from '@/composables/useClipboard'; +import { waitFor } from '@testing-library/vue'; vi.mock('vue-router', async (importOriginal) => { const actual = await importOriginal<{}>(); @@ -74,68 +75,89 @@ describe('useCanvasOperations', () => { workflowsStore.resetWorkflow(); workflowsStore.resetState(); - await workflowHelpers.initState(workflow); + workflowHelpers.initState(workflow); canvasOperations = useCanvasOperations({ router, lastClickPosition }); vi.clearAllMocks(); }); - describe('addNode', () => { - it('should throw error when node type does not exist', async () => { - vi.spyOn(nodeTypesStore, 'getNodeTypes').mockResolvedValue(undefined); + describe('requireNodeTypeDescription', () => { + it('should return node type description when type and version match', () => { + const type = 'testType'; + const version = 1; + const expectedDescription = mockNodeTypeDescription({ name: type, version }); + nodeTypesStore.setNodeTypes([expectedDescription]); - await expect(canvasOperations.addNode({ type: 'nonexistent' })).rejects.toThrow(); + const result = canvasOperations.requireNodeTypeDescription(type, version); + + expect(result).toBe(expectedDescription); }); - it('should create node with default version when version is undefined', async () => { - nodeTypesStore.setNodeTypes([mockNodeTypeDescription({ name: 'type' })]); + it('should throw an error when node type does not exist', () => { + const type = 'nonexistentType'; - const result = await canvasOperations.addNode({ - name: 'example', - type: 'type', - }); + expect(() => { + canvasOperations.requireNodeTypeDescription(type); + }).toThrow(); + }); + + it('should return node type description when only type is provided and it exists', () => { + const type = 'testTypeWithoutVersion'; + const expectedDescription = mockNodeTypeDescription({ name: type }); + nodeTypesStore.setNodeTypes([expectedDescription]); + + const result = canvasOperations.requireNodeTypeDescription(type); + + expect(result).toBe(expectedDescription); + }); + }); + + describe('addNode', () => { + it('should create node with default version when version is undefined', () => { + const result = canvasOperations.addNode( + { + name: 'example', + type: 'type', + typeVersion: 1, + }, + mockNodeTypeDescription({ name: 'type' }), + ); expect(result.typeVersion).toBe(1); }); - it('should create node with last version when version is an array', async () => { - nodeTypesStore.setNodeTypes([mockNodeTypeDescription({ name: 'type', version: [1, 2] })]); - - const result = await canvasOperations.addNode({ - type: 'type', - }); - - expect(result.typeVersion).toBe(2); - }); - - it('should create node with default position when position is not provided', async () => { - nodeTypesStore.setNodeTypes([mockNodeTypeDescription({ name: 'type' })]); - - const result = await canvasOperations.addNode({ - type: 'type', - }); + it('should create node with default position when position is not provided', () => { + const result = canvasOperations.addNode( + { + type: 'type', + typeVersion: 1, + }, + mockNodeTypeDescription({ name: 'type' }), + ); expect(result.position).toEqual([460, 460]); // Default last click position }); - it('should create node with provided position when position is provided', async () => { - nodeTypesStore.setNodeTypes([mockNodeTypeDescription({ name: 'type' })]); - - const result = await canvasOperations.addNode({ - type: 'type', - position: [20, 20], - }); + it('should create node with provided position when position is provided', () => { + const result = canvasOperations.addNode( + { + type: 'type', + typeVersion: 1, + position: [20, 20], + }, + mockNodeTypeDescription({ name: 'type' }), + ); expect(result.position).toEqual([20, 20]); }); - it('should create node with default credentials when only one credential is available', async () => { + it('should create node with default credentials when only one credential is available', () => { const credential = mock({ id: '1', name: 'cred', type: 'cred' }); const nodeTypeName = 'type'; - - nodeTypesStore.setNodeTypes([ - mockNodeTypeDescription({ name: nodeTypeName, credentials: [{ name: credential.name }] }), - ]); + const nodeTypeDescription = mockNodeTypeDescription({ + name: nodeTypeName, + credentials: [{ name: credential.name }], + }); credentialsStore.addCredentials([credential]); @@ -144,24 +166,25 @@ describe('useCanvasOperations', () => { credential, ]); - const result = await canvasOperations.addNode({ - type: nodeTypeName, - }); + const result = canvasOperations.addNode( + { + type: nodeTypeName, + typeVersion: 1, + }, + nodeTypeDescription, + ); expect(result.credentials).toEqual({ [credential.name]: { id: '1', name: credential.name } }); }); - it('should not assign credentials when multiple credentials are available', async () => { + it('should not assign credentials when multiple credentials are available', () => { const credentialA = mock({ id: '1', name: 'credA', type: 'cred' }); const credentialB = mock({ id: '1', name: 'credB', type: 'cred' }); const nodeTypeName = 'type'; - - nodeTypesStore.setNodeTypes([ - mockNodeTypeDescription({ - name: nodeTypeName, - credentials: [{ name: credentialA.name }, { name: credentialB.name }], - }), - ]); + const nodeTypeDescription = mockNodeTypeDescription({ + name: nodeTypeName, + credentials: [{ name: credentialA.name }, { name: credentialB.name }], + }); // @ts-expect-error Known pinia issue when spying on store getters vi.spyOn(credentialsStore, 'getUsableCredentialByType', 'get').mockReturnValue(() => [ @@ -169,24 +192,30 @@ describe('useCanvasOperations', () => { credentialB, ]); - const result = await canvasOperations.addNode({ - type: 'type', - }); + const result = canvasOperations.addNode( + { + type: 'type', + typeVersion: 1, + }, + nodeTypeDescription, + ); expect(result.credentials).toBeUndefined(); }); it('should open NDV when specified', async () => { - nodeTypesStore.setNodeTypes([mockNodeTypeDescription({ name: 'type' })]); + const nodeTypeDescription = mockNodeTypeDescription({ name: 'type' }); - await canvasOperations.addNode( + canvasOperations.addNode( { type: 'type', + typeVersion: 1, name: 'Test Name', }, + nodeTypeDescription, { openNDV: true }, ); - expect(ndvStore.activeNodeName).toBe('Test Name'); + await waitFor(() => expect(ndvStore.activeNodeName).toBe('Test Name')); }); }); @@ -719,7 +748,7 @@ describe('useCanvasOperations', () => { const addConnectionSpy = vi.spyOn(workflowsStore, 'addConnection'); - await canvasOperations.addConnections(connections); + canvasOperations.addConnections(connections); expect(addConnectionSpy).toHaveBeenCalledWith({ connection: [ diff --git a/packages/editor-ui/src/composables/useCanvasOperations.ts b/packages/editor-ui/src/composables/useCanvasOperations.ts index 1771e922a0..6671da520f 100644 --- a/packages/editor-ui/src/composables/useCanvasOperations.ts +++ b/packages/editor-ui/src/composables/useCanvasOperations.ts @@ -97,6 +97,10 @@ type AddNodeData = Partial & { type: string; }; +type AddNodeDataWithTypeVersion = AddNodeData & { + typeVersion: INodeUi['typeVersion']; +}; + type AddNodeOptions = { dragAndDrop?: boolean; openNDV?: boolean; @@ -413,6 +417,18 @@ export function useCanvasOperations({ historyStore.stopRecordingUndo(); } + function requireNodeTypeDescription(type: INodeUi['type'], version?: INodeUi['typeVersion']) { + const nodeTypeDescription = nodeTypesStore.getNodeType(type, version); + if (!nodeTypeDescription) { + throw new Error( + i18n.baseText('nodeView.showMessage.addNodeButton.message', { + interpolate: { nodeTypeName: type }, + }), + ); + } + return nodeTypeDescription; + } + async function addNodes( nodes: AddedNodesAndConnections['nodes'], options: { @@ -420,6 +436,7 @@ export function useCanvasOperations({ position?: XYPosition; trackHistory?: boolean; trackBulk?: boolean; + keepPristine?: boolean; } = {}, ) { let insertPosition = options.position; @@ -429,16 +446,32 @@ export function useCanvasOperations({ historyStore.startRecordingUndo(); } - for (const nodeAddData of nodes) { + const nodesWithTypeVersion = nodes.map((node) => { + const typeVersion = resolveNodeVersion(requireNodeTypeDescription(node.type)); + return { + ...node, + typeVersion, + }; + }); + + await loadNodeTypesProperties(nodesWithTypeVersion); + + if (options.trackBulk) { + historyStore.startRecordingUndo(); + } + + for (const nodeAddData of nodesWithTypeVersion) { const { isAutoAdd, openDetail: openNDV, ...node } = nodeAddData; const position = node.position ?? insertPosition; + const nodeTypeDescription = requireNodeTypeDescription(node.type, node.typeVersion); try { - lastAddedNode = await addNode( + lastAddedNode = addNode( { ...node, position, }, + nodeTypeDescription, { ...options, openNDV, @@ -459,13 +492,16 @@ export function useCanvasOperations({ } if (lastAddedNode) { - // @TODO Figure out what this does and why it's needed updatePositionForNodeWithMultipleInputs(lastAddedNode); } if (options.trackBulk) { historyStore.stopRecordingUndo(); } + + if (!options.keepPristine) { + uiStore.stateIsDirty = true; + } } function updatePositionForNodeWithMultipleInputs(node: INodeUi) { @@ -484,16 +520,11 @@ export function useCanvasOperations({ } } - async function addNode(node: AddNodeData, options: AddNodeOptions = {}): Promise { - const nodeTypeDescription = nodeTypesStore.getNodeType(node.type); - if (!nodeTypeDescription) { - throw new Error( - i18n.baseText('nodeView.showMessage.addNodeButton.message', { - interpolate: { nodeTypeName: node.type }, - }), - ); - } - + function addNode( + node: AddNodeDataWithTypeVersion, + nodeTypeDescription: INodeTypeDescription, + options: AddNodeOptions = {}, + ): INodeUi { // Check if maximum allowed number of this type of node has been reached if ( nodeTypeDescription.maxNodes !== undefined && @@ -507,35 +538,32 @@ export function useCanvasOperations({ ); } - const nodeData = await resolveNodeData(node, nodeTypeDescription); + const nodeData = resolveNodeData(node, nodeTypeDescription); if (!nodeData) { throw new Error(i18n.baseText('nodeViewV2.showError.failedToCreateNode')); } - historyStore.startRecordingUndo(); - if (options.trackHistory) { - historyStore.pushCommandToUndo(new AddNodeCommand(nodeData)); - } - - workflowsStore.addNode(nodeData); nodeHelpers.matchCredentials(nodeData); - if (!options.isAutoAdd) { - createConnectionToLastInteractedWithNode(nodeData, options); - } + void nextTick(() => { + if (options.trackHistory) { + historyStore.pushCommandToUndo(new AddNodeCommand(nodeData)); + } - runAddNodeHooks(nodeData, options); + workflowsStore.addNode(nodeData); - historyStore.stopRecordingUndo(); + if (!options.isAutoAdd) { + createConnectionToLastInteractedWithNode(nodeData, options); + } + + runAddNodeHooks(nodeData, options); + + if (options.openNDV) { + ndvStore.setActiveNodeName(nodeData.name); + } + }); workflowsStore.setNodePristine(nodeData.name, true); - uiStore.stateIsDirty = true; - - if (options.openNDV) { - void nextTick(() => { - ndvStore.setActiveNodeName(nodeData.name); - }); - } return nodeData; } @@ -661,11 +689,14 @@ export function useCanvasOperations({ /** * Resolves the data for a new node */ - async function resolveNodeData(node: AddNodeData, nodeTypeDescription: INodeTypeDescription) { + function resolveNodeData( + node: AddNodeDataWithTypeVersion, + nodeTypeDescription: INodeTypeDescription, + ) { const id = node.id ?? uuid(); const name = node.name ?? (nodeTypeDescription.defaults.name as string); const type = nodeTypeDescription.name; - const typeVersion = resolveNodeVersion(nodeTypeDescription); + const typeVersion = node.typeVersion; const position = resolveNodePosition(node as INodeUi, nodeTypeDescription); const disabled = node.disabled ?? false; const parameters = node.parameters ?? {}; @@ -681,8 +712,6 @@ export function useCanvasOperations({ parameters, }; - await loadNodeTypesProperties([{ name: nodeData.type, version: nodeData.typeVersion }]); - resolveNodeParameters(nodeData); resolveNodeCredentials(nodeData, nodeTypeDescription); resolveNodeName(nodeData); @@ -691,7 +720,9 @@ export function useCanvasOperations({ return nodeData; } - async function loadNodeTypesProperties(nodeInfos: INodeTypeNameVersion[]): Promise { + async function loadNodeTypesProperties( + nodes: Array>, + ): Promise { const allNodeTypeDescriptions: INodeTypeDescription[] = nodeTypesStore.allNodeTypes; const nodesToBeFetched: INodeTypeNameVersion[] = []; @@ -700,8 +731,8 @@ export function useCanvasOperations({ ? nodeTypeDescription.version : [nodeTypeDescription.version]; if ( - !!nodeInfos.find( - (n) => n.name === nodeTypeDescription.name && nodeVersions.includes(n.version), + !!nodes.find( + (n) => n.type === nodeTypeDescription.name && nodeVersions.includes(n.typeVersion), ) && !nodeTypeDescription.hasOwnProperty('properties') ) { @@ -1154,7 +1185,7 @@ export function useCanvasOperations({ return false; } - async function addConnections(connections: CanvasConnectionCreateData[] | CanvasConnection[]) { + function addConnections(connections: CanvasConnectionCreateData[] | CanvasConnection[]) { for (const { source, target, data } of connections) { createConnection({ source, @@ -1211,10 +1242,10 @@ export function useCanvasOperations({ async function initializeWorkspace(data: IWorkflowDb) { // Set workflow data - await workflowHelpers.initState(data); + workflowHelpers.initState(data); // Add nodes and connections - await addNodes(data.nodes); + await addNodes(data.nodes, { keepPristine: true }); workflowsStore.setConnections(data.connections); } @@ -1383,7 +1414,7 @@ export function useCanvasOperations({ historyStore.startRecordingUndo(); await addNodes(Object.values(tempWorkflow.nodes)); - await addConnections( + addConnections( mapLegacyConnectionsToCanvasConnections( tempWorkflow.connectionsBySourceNode, Object.values(tempWorkflow.nodes), @@ -1668,6 +1699,7 @@ export function useCanvasOperations({ editableWorkflow, editableWorkflowObject, triggerNodes, + requireNodeTypeDescription, addNodes, addNode, revertAddNode, diff --git a/packages/editor-ui/src/composables/useWorkflowHelpers.ts b/packages/editor-ui/src/composables/useWorkflowHelpers.ts index b78c030180..7a317110c1 100644 --- a/packages/editor-ui/src/composables/useWorkflowHelpers.ts +++ b/packages/editor-ui/src/composables/useWorkflowHelpers.ts @@ -1055,7 +1055,7 @@ export function useWorkflowHelpers(options: { router: ReturnType { + function initState(workflowData: IWorkflowDb) { workflowsStore.addWorkflow(workflowData); workflowsStore.setActive(workflowData.active || false); workflowsStore.setWorkflowId(workflowData.id); diff --git a/packages/editor-ui/src/views/NodeView.v2.vue b/packages/editor-ui/src/views/NodeView.v2.vue index df83466fdd..696af42c47 100644 --- a/packages/editor-ui/src/views/NodeView.v2.vue +++ b/packages/editor-ui/src/views/NodeView.v2.vue @@ -825,9 +825,11 @@ async function onAddNodesAndConnections( }; }); - await addConnections(mappedConnections); + addConnections(mappedConnections); - uiStore.resetLastInteractedWith(); + void nextTick(() => { + uiStore.resetLastInteractedWith(); + }); } async function onRevertAddNode({ node }: { node: INodeUi }) { diff --git a/packages/editor-ui/src/views/NodeView.vue b/packages/editor-ui/src/views/NodeView.vue index cd4433c669..fbb32faaad 100644 --- a/packages/editor-ui/src/views/NodeView.vue +++ b/packages/editor-ui/src/views/NodeView.vue @@ -1389,7 +1389,7 @@ export default defineComponent({ this.resetWorkspace(); - await this.workflowHelpers.initState(workflow); + this.workflowHelpers.initState(workflow); if (workflow.sharedWithProjects) { this.workflowsEEStore.setWorkflowSharedWith({ diff --git a/packages/editor-ui/src/views/WorkflowExecutionsView.vue b/packages/editor-ui/src/views/WorkflowExecutionsView.vue index 567e020fe7..3cca54466e 100644 --- a/packages/editor-ui/src/views/WorkflowExecutionsView.vue +++ b/packages/editor-ui/src/views/WorkflowExecutionsView.vue @@ -125,7 +125,7 @@ async function fetchWorkflow() { try { await workflowsStore.fetchActiveWorkflows(); const data = await workflowsStore.fetchWorkflow(workflowId.value); - await workflowHelpers.initState(data); + workflowHelpers.initState(data); await nodeHelpers.addNodes(data.nodes, data.connections); } catch (error) { toast.showError(error, i18n.baseText('nodeView.showError.openWorkflow.title'));