Refactoring the `RED.nodes.import()` function #4757

GogoVega · 2024-06-11T09:21:34Z

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Proposed changes

Refactoring the importNodes function.

Resolves also:

Bad number of node.users for Config Node:
- Deleting a Node inside a Subflow does not update the number of users.
- Replacing a Config Node overwrites the user list to empty.
A Node that has an invalid number of inputs/outputs (dynamic number) is not correctly handled.
When copying a Subflow (tab), the name is not updated (addition of the copy count)
When importing a Subflow:
- If the definition is missing, the Node will be marked as unknown - which allows to double click and have dialog box and be able to export this Node.
- When adding the definition, existing instances into the workspace are updated.
- A Node missing into the Subflow import is now handled (to avoid create link)
Ensure activeWorkspace can no longer be equal to 0.

Other functions modified:

updateConfigNodeUsers - for either add or remove and either trigger the event
RED.view.importNodes() - overwrite importMap to undefined

Checklist

I have read the contribution guidelines
For non-bugfix PRs, I have discussed this change on the forum/slack team.
I have run npm run test to verify the unit tests pass
I have added suitable unit tests to cover the new/changed functionality

GogoVega

Thanks for your answers.

GogoVega · 2024-06-11T09:47:16Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

                    // No conflict resolution for this node
-                    var existing = allNodes.getNode(id) || configNodes[id] || workspaces[id] || subflows[id] || groups[id] || junctions[id];
-                    if (existing) {
-                        existingNodes.push({existing:existing, imported:n});
+                    // TODO: Sure?


Is the comment correct? There is a conflict because it's an existing node.

packages/node_modules/@node-red/editor-client/src/js/nodes.js

GogoVega · 2024-06-11T10:03:53Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

        }

+        // TODO: Right place?
        RED.workspaces.refresh();


Right place?

packages/node_modules/@node-red/editor-client/src/js/nodes.js

GogoVega · 2024-06-11T10:15:02Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

+                if (activeWorkspace === 0) {    // TODO: Why?
+                    activeWorkspace = node.id;
                }


In which case?

knolleary · 2024-06-11T20:39:49Z

This function is so central to the editor, I am wary of anyone else doing a large refactoring of it. It contains 10 years of built up logic, and in many cases there are good reasons for the order things are done in.

We are also trying to get 4.0 shipped. I simply don't have bandwidth to give this work the attention it needs any time soon given there is no compelling reason to do it.

I will try to take a look at some point, but I can't say when.

GogoVega · 2024-06-12T20:01:00Z

No worries, focus on v4 and for my part I'll continue here - It's going to take me a while, then test it thoroughly.

Given all that this involves, I had not planned to bring it in for v4. So come back here as soon as you can but without pressure. Thanks 😉

…ting it

GogoVega · 2024-06-24T11:25:15Z

Do we still have to handle the import of singleton config node? If so, the only currently unhandled case would be that multiple nodes use the same config node - this config node will need to be copied and re-associated with each node.

GogoVega

Hi Nick,
These points remain to be discussed.

Tell me if you want me to target the code for fixes to help you review this very long PR?

GogoVega · 2024-06-26T07:30:00Z

packages/node_modules/@node-red/editor-client/src/js/ui/subflow.js

-            }
-            if (n.z == id) {
+            if (n.z === id || !keepInstanceNodes && n.type === "subflow:" + id) {
+                RED.nodes.updateConfigNodeUsers(n, { emitEvent: false, action: "remove" });


The event must (should) be triggered.

Suggested change

RED.nodes.updateConfigNodeUsers(n, { emitEvent: false, action: "remove" });

RED.nodes.updateConfigNodeUsers(n, { action: "remove" });

GogoVega · 2024-06-26T07:33:06Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

+                // If 'wires' is longer than outputs, clip wires
+                console.log("Warning: node.wires longer than node.outputs - trimming wires:", node.id, " wires:", node.wires.length, " outputs:", node.outputs);
+                // TODO: Pas dans l'autre sens ?
+                newNode.wires = newNode.wires.slice(0, newNode.outputs);


I would remove the last elements instead of the first.

GogoVega · 2024-06-26T07:35:40Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

+            addWorkspace(recoveryWorkspace);
+            RED.workspaces.add(recoveryWorkspace);
+            // Put the recovery workspace at the first position
+            newWorkspaces.splice(0, 1, recoveryWorkspace, newWorkspaces[0]);


Can be improved by:

Suggested change

newWorkspaces.splice(0, 1, recoveryWorkspace, newWorkspaces[0]);

newWorkspaces.unshift(recoveryWorkspace);

GogoVega · 2024-06-26T07:37:46Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

+            let def = registry.getNodeType(node.type);
+
+            // Update the Node definition for Subflow instance
+            // TODO: A thing with `node.i`


See what node.i is still used for

GogoVega · 2024-06-26T07:39:35Z

packages/node_modules/@node-red/editor-client/src/js/nodes.js

-                         node.outputs = 1
-                         node.w = 0;
-                         node.h = 0;
+            // TODO: Group Node has config as category - why?


The group def is really strange - should is in the types like the others but that's not the point of this PR.

knolleary · 2024-06-26T08:20:32Z

To be absolutely direct - as I was in an earlier comment - I'm not sure how to move forward with this PR. It is massive and changes such an important part of the editor code base. The PR not only moves code around, but also changes the logic in different places due to the various fixes you've identified. So not only do I have to understand how you've moved the code around, I need to understand why you've changed any particular bits of logic - which could be a bug you've fixed or a bug you've introduced.

I simply don't know when I will have the time needed to focus on this.

GogoVega added 3 commits June 7, 2024 15:09

Initial commit of function importNodes refactoring

b495c18

Second commit of importNodes refactoring

7e69eab

Merge branch 'master' into importNodes-refactoring

a67e89e

GogoVega commented Jun 11, 2024

View reviewed changes

Fix bad use of hasOwnProperty on an Array

39d8386

GogoVega added 12 commits June 15, 2024 11:39

Fix importing a node with an invalid number of outputs.

e5db666

Mainly refactor the copy of subflow + slight cleanup

5070389

Cleanup and remove old logic when importing identical subflows

4f20cd5

Fix adding links to missing nodes inside a subflow

778e3ae

Fix number of users when deleting nodes

2f01fe5

Change the name of getID by generateId

476e834

Fix updating Subflow name when it's a copy

58d4c36

Fix number of users when replacing config nodes

ee295e3

Fix replacement of unknown subflow instance and cleanup

e6fa051

Fix Group import and user deletion (bad object reference)

5d78c54

Doc the (new) functions and add the onNodeTypeAdded function

6a2a763

Fix replacing an unknown node does not delete the node before reimpor…

3433961

…ting it

GogoVega commented Jun 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring the `RED.nodes.import()` function #4757

Refactoring the `RED.nodes.import()` function #4757

GogoVega commented Jun 11, 2024 •

edited

Loading

GogoVega left a comment

GogoVega Jun 11, 2024

GogoVega Jun 11, 2024

GogoVega Jun 11, 2024

knolleary commented Jun 11, 2024

GogoVega commented Jun 12, 2024

GogoVega commented Jun 24, 2024

GogoVega left a comment

GogoVega Jun 26, 2024

GogoVega Jun 26, 2024

GogoVega Jun 26, 2024

GogoVega Jun 26, 2024

GogoVega Jun 26, 2024

knolleary commented Jun 26, 2024

	RED.nodes.updateConfigNodeUsers(n, { emitEvent: false, action: "remove" });
	RED.nodes.updateConfigNodeUsers(n, { action: "remove" });

	newWorkspaces.splice(0, 1, recoveryWorkspace, newWorkspaces[0]);
	newWorkspaces.unshift(recoveryWorkspace);

Refactoring the RED.nodes.import() function #4757

Are you sure you want to change the base?

Refactoring the RED.nodes.import() function #4757

Conversation

GogoVega commented Jun 11, 2024 • edited Loading

Proposed changes

Checklist

GogoVega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knolleary commented Jun 11, 2024

GogoVega commented Jun 12, 2024

GogoVega commented Jun 24, 2024

GogoVega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knolleary commented Jun 26, 2024

Refactoring the `RED.nodes.import()` function #4757

Refactoring the `RED.nodes.import()` function #4757

GogoVega commented Jun 11, 2024 •

edited

Loading