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): Drop outgoing connections on order changed event for nodes with dynamic outputs #9055

Conversation

michael-radency
Copy link
Contributor

@michael-radency michael-radency commented Apr 4, 2024

Summary

Because of Switch outputs are configured dynamically, based on rules, we are disconnecting connected nodes on rules reorder and show a warning in UI

image

Related tickets and issues

https://linear.app/n8n/issue/NODE-1273/switch-reordering-changes-which-nodes-are-connected-to-which-outputs
https://community.n8n.io/t/reorder-outputs-in-switch-does-not-reorder-output-connections/43302

@michael-radency michael-radency added ui Enhancement in /editor-ui or /design-system n8n team Authored by the n8n team node/issue Issue with a node labels Apr 4, 2024
@michael-radency michael-radency changed the title fix: Drop outgoing connections on order changed event fix: Drop outgoing connections on order changed event for nodes with dynamic outputs Apr 4, 2024
…switch-reordering-changes-which-nodes-are-connected-to-which
Copy link
Contributor

@OlegIvaniv OlegIvaniv left a comment

Choose a reason for hiding this comment

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

Could you also please add a small test to check that nodes are actually disconnected when you change the Swithc node output order? 🙏

@@ -853,6 +857,20 @@ export default defineComponent({
return;
}

if (
parameterData.type &&
SHOULD_CLEAR_NODE_OUTPUTS[nodeType.name] &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Eslint warning here: Prefer using an optional chain expression instead, as it's more concise and easier to read.eslint[@typescript-eslint/prefer-optional-chain](https://typescript-eslint.io/rules/prefer-optional-chain)

Comment on lines 203 to 212
clearNodeOutgoingConnections() {
const uiStore = useUIStore();
return (nodeName: string): void => {
if (this.workflow.connections.hasOwnProperty(nodeName)) {
this.workflow.connections[nodeName] = {};
uiStore.stateIsDirty = true;
return;
}
};
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be a getter but rather an action

@@ -1093,6 +1093,8 @@
"nodeSettings.latest": "Latest",
"nodeSettings.deprecated": "Deprecated",
"nodeSettings.latestVersion": "Latest version: {version}",
"nodeSettings.outputCleared.title": "Parameters changed",
"nodeSettings.outputCleared.message": "Order of parameters changed, outgoing connections was cleared",
Copy link
Contributor

Choose a reason for hiding this comment

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

Order of parameters changed, outgoing connections were cleared.

…switch-reordering-changes-which-nodes-are-connected-to-which
…switch-reordering-changes-which-nodes-are-connected-to-which
…switch-reordering-changes-which-nodes-are-connected-to-which
@OlegIvaniv OlegIvaniv changed the title fix: Drop outgoing connections on order changed event for nodes with dynamic outputs fix(editor): Drop outgoing connections on order changed event for nodes with dynamic outputs Apr 8, 2024
Copy link
Contributor

@OlegIvaniv OlegIvaniv left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all my points, LGTM!

Copy link
Contributor

github-actions bot commented Apr 8, 2024

✅ All Cypress E2E specs passed

Copy link

cypress bot commented Apr 8, 2024

2 flaky tests on run #4625 ↗︎

0 354 12 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 michael-radency 🗃️ e2e/*
Project: n8n Commit: e835c2afea
Status: Passed Duration: 04:22 💡
Started: Apr 8, 2024 8:58 PM Ended: Apr 8, 2024 9:02 PM
Flakiness  cypress/e2e/5-ndv.cy.ts • 2 flaky tests

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Screenshots Video
NDV > Stop listening for trigger event from NDV Screenshots Video

Review all test suite changes for PR #9055 ↗︎

@michael-radency michael-radency merged commit 3dd70a1 into master Apr 9, 2024
30 checks passed
@michael-radency michael-radency deleted the node-1273-switch-reordering-changes-which-nodes-are-connected-to-which branch April 9, 2024 01:46
@github-actions github-actions bot mentioned this pull request Apr 10, 2024
@janober
Copy link
Member

janober commented Apr 10, 2024

Got released with n8n@1.37.0

netroy pushed a commit that referenced this pull request Apr 11, 2024
@github-actions github-actions bot mentioned this pull request Apr 11, 2024
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 node/issue Issue with a node Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants