chore: Node Definition Cleanup

[camielvs]

Description

Moves around a bit of code - particularly for Node Definition and types in React Flow - to standardise and tidy things in advance of Flex (sticky note) Nodes.

This includes the explicit provision of readOnly mode to created nodes (flex nodes will make use of this).

Also moves the Common Annotations list fallback to a more suitable location (to avoid flaky circular dependencies). And fixes a typo in buildTaskSpecShape.

No change to UI.

Related Issue and Pull requests

Type of Change

• Cleanup/Refactor

Checklist

• I have tested this does not break current pipelines / runs functionality
• I have tested the changes on staging

Screenshots (if applicable)

Test Instructions

Confirm the app still works as expected.

Additional Comments

[camielvs]

• feat: Add StackingControls for Managing zIndex #1756 : 2 dependent PRs (#1728 , #1758 )
• chore: Node Definition Cleanup #1731 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[maxy-shpfy]

NIT: Naming is little bit confusing: in this file we have TaskType (task / input / output) - which technically is a "NodeType". Task is a member of tasks collection and is reflection of TaskSpec. While inputs and outputs are members of different collections with different shapes.

So TaskType -- isTask_Node_Type - I see names here also are not aligned.

NodeType as *keyof* available react flow node types. nodeTypes is a dict to match name to a component... maybe we should call it supportedNodeTypeComponents or nodeComponentsRegistry? Discussion is open.

[camielvs]

I agree. I do not like that TaskNodeData is overloaded with Input and Output Node data as well. This is something that was addressed in #928 as part of the foundations for the Node Manager, but sadly it was not shipped even though it has wider relevance outside of the Node Manager work.

I don't think this is the PR or place to go through such a rework again. But I can look at #928 again and reopen, if you'd like to see things cleaned up?

[maxy-shpfy]

Just wondering, why we drilling readonly prop thru the stack, instead of making it a property of TaskNodeData structure?

[camielvs]

^ it already is a property of TaskNodeData. However, I opted to keep it separate in advance of flex nodes which will not use TaskNodeData and so hence will need readOnly explicitly passed down anyway.

Now I think about it more it seems kind of silly to have the property there and not use it. But then it also makes me wonder if we should be supplying readOnly state to the page via a provider rather than passing it manually through nodes and other data.

[camielvs]

Merge activity

• Feb 19, 7:57 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Feb 19, 7:57 PM UTC: @camielvs merged this pull request with Graphite.