Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(editor): Fix merge node connectors #5364

Merged
merged 2 commits into from
Feb 6, 2023
Merged

Conversation

OlegIvaniv
Copy link
Contributor

@OlegIvaniv OlegIvaniv commented Feb 5, 2023

In #5259, we've removed workflowsStore.addConnection call after EVENT_CONNECTION. This resulted in merge node connections not being stored. This PR reverts those changes.
Github issue / Community forum post (link here to close automatically): #5359

@OlegIvaniv OlegIvaniv requested a review from cstuncsik February 5, 2023 15:56
@OlegIvaniv
Copy link
Contributor Author

@cstuncsik Do you remember what was the rationale behind removing this call in #5259? Could you please review this PR as it's related?

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Feb 5, 2023
@@ -2159,6 +2159,10 @@ export default mixins(
},
];

this.workflowsStore.addConnection({
Copy link
Contributor

Choose a reason for hiding this comment

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

@OlegIvaniv as I remember the event connection callback that calls workflowsStore.addConnection is called also when you load a module and by passing setStateDirty: true the module immediately gets into dirty state and you get the unsaved change confirmation modal when you try to navigate away without editing the workflow.

Also workflowsStore.addConnection is called elsewhere in the process of connecting nodes and I thought it is unnecessary here.

Maybe this just should not change setStateDirty just use the value it has at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we set up input endpoints we set target like this:

target: !this.isReadOnly && nodeTypeData.inputs.length > 1, // only enabled for nodes with multiple inputs.. otherwise attachment handled by connectionDrag event in NodeView,

That's why we can't rely only on EVENT_CONNECTION_ABORT hook to call connectTwoNodes which calls this.workflowsStore.addConnection because for nodes that have more than 1 input(merge node) we have to use EVENT_CONNECTION hook. So I've added this call to EVENT_CONNECTION and manually set uiStore.stateIsDirty = true

@cstuncsik cstuncsik self-requested a review February 6, 2023 08:03
@OlegIvaniv OlegIvaniv merged commit 20356ba into master Feb 6, 2023
@OlegIvaniv OlegIvaniv deleted the fix_merge_node_connections branch February 6, 2023 08:49
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Feb 6, 2023
@janober
Copy link
Member

janober commented Feb 6, 2023

Got released with n8n@0.214.1

@janober janober removed the Upcoming Release Will be part of the upcoming release label Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants