Skip to content

Conversation

GogoVega
Copy link
Contributor

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

Proposed changes

Part of #4757.

This first part of the importNodes refactoring moves some logic into functions:

  • identifyUnknownTypes
  • copyConfigNode
  • copyNode

identifyUnknownTypes adds a loop in this PR but another one will disappear in the future.

I haven't fixed any issues (added todo flag) except in one place: L2469 if the id is updated at this line, the condition at line L2492 will no longer be true. Import copy force generateIds to true that's why never detected before. But I wonder if it is not the subflow id that should be used instead of node id.

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
// TODO: Group Node has config as category - why?
const isConfigNode = def?.category === "config" && n.type !== "group";
if (isConfigNode) {
node_map[n.id] = copyConfigNode(n, def, options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
node_map[n.id] = copyConfigNode(n, def, options);
node_map[n.id] = copyConfigNode(n, def, options);
delete node_map[n.id]._configNodeReferences;

if (subflow) {
// If the parent Subflow is found, update Subflow instance properties
if (subflow_denylist[parentId] || createNewIds || options.importMap[n.id] === "copy") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the Subflow definition was copied, subflow_map[parentId] contains it.
It's parentId that should be used in importMap to update the type to the copied Subflow definition.

Suggested change
if (subflow_denylist[parentId] || createNewIds || options.importMap[n.id] === "copy") {
if (subflow_denylist[parentId] || createNewIds || options.importMap[parentId] === "copy") {
Comment on lines +2456 to +2457
// TODO: If updated here, conflict with importMap in L2634
//n.id = getID();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's handled at the end, we can remove it there.

Suggested change
// TODO: If updated here, conflict with importMap in L2634
//n.id = getID();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant