refactor(ui): WorkflowDag component from class to functional #11920

tetora1053 · 2023-09-30T13:53:34Z

Partial fix for #9810

39. ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx:

Motivation

Use newer, modern functional components compatible with hooks instead of legacy class-based components

Modifications

this.state -> useState
this.props -> props
methods -> inner functions
refactoring for readability

Verification

Mostly no real semantic changes
manually tested in local environment

Reference PRs

Signed-off-by: tetora1053 <tetora1053@gmail.com>

tetora1053 · 2023-09-30T16:21:54Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

-    private selectNode(nodeId: string) {
-        if (isCollapsedNode(nodeId)) {
-            this.expandNode(nodeId);
-        } else {
-            return this.props.nodeClicked && this.props.nodeClicked(nodeId);
-        }
-    }


I inlined this function as it's just used here.

tetora1053 · 2023-09-30T16:27:26Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

-    private saveOptions(newChanges: WorkflowDagRenderOptions) {
-        localStorage.setItem('showArtifacts', newChanges.showArtifacts ? 'true' : 'false');
-        this.setState(newChanges);


also inlined this function

tetora1053 · 2023-09-30T16:33:01Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx


-        interface PrepareNode {


I moved this definition as I wanted to put interfaces together in one place.

agilgur5

Was sick for a bit, so just getting to more complex reviews now. Turns out this isn't too complex, just that the diff is wacky due to the re-orderings 😅 It was a lot easier to review with my editor open with old version on one screen and the new code in another screen

just a handful of comments below, otherwise confirmed that everything else was semantically equivalent

agilgur5 · 2023-10-13T04:08:05Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

-    private saveOptions(newChanges: WorkflowDagRenderOptions) {
-        localStorage.setItem('showArtifacts', newChanges.showArtifacts ? 'true' : 'false');
-        this.setState(newChanges);
+    prepareGraph();


this looks like it would be called twice on render now -- once on each, and then another time if the below effect's dependencies change. was there a reason for that, or just old in-progress code?

it may make sense to make graph into a ref, so that it's preserved between renders.

or maybe even better -- perhaps we could convert the effect to a useMemo?

Yeah it looks even better when it's memoized!
I've memoized graph inlining prepareGraph func into callback func.
and removed useEffect

agilgur5 · 2023-10-13T04:10:11Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

-        if (!node) {
-            return null;
+    function prepareGraph() {
+        const {edges, nodes} = (graph = new Graph());
+
+        if (!props.nodes) {
+            return;
        }


was this not needed? the check does still exist in a few places

oh this comment ended up a bit weird (turned on "ignore whitespace" in GH) -- I was commenting on the if (!node) check that existed in the getNode function

oh I see, it already returns undefined if it doesn't exist, so yea, this isn't necessary. good simplification!
there were a few of these changes that took a bit for my brain to realize they're semantically equivalent.

so nvm on this comment thread 😄

agilgur5 · 2023-10-13T04:12:09Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

@@ -210,103 +221,77 @@ export class WorkflowDag extends React.Component<WorkflowDagProps, WorkflowDagRe
                }
                consideredChildren.add(item.nodeName);

-                const node: NodeStatus = this.props.nodes[item.nodeName];
+                const node = props.nodes[item.nodeName];


Suggested change

const node = props.nodes[item.nodeName];

const node = getNode(item.nodeName);

for consistency with the existing code, since we're already modifying it

agilgur5 · 2023-10-13T04:14:28Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

-        if (!node) {
-            return null;
+    function prepareGraph() {
+        const {edges, nodes} = (graph = new Graph());


Suggested change

const {edges, nodes} = (graph = new Graph());

graph = new Graph();

const {edges, nodes} = graph;

a bit less confusing syntax. also more conventional for JS

this will end up changing a bit with a useMemo that I mentioned above

agilgur5 · 2023-10-13T04:24:11Z

ui/src/app/workflows/components/workflow-dag/workflow-dag.tsx

-            />
-        );
-    }
+    const [expandNodes, setExpandNodes] = useState(new Set(''));


was there a reason you gave it the default '', different from the previous version which had no default?

In previous version,expandNodes would be of type unknown.
I wanted to gave it a more specific type, so assigned a default value.

now I feel it might be better to use generics.
const [expandNodes, setExpandNodes] = useState(new Set<string>());

I decided to use generics here!

agilgur5 · 2023-10-28T01:34:36Z

@tetora1053 have you had a chance to follow-up on this?

tetora1053 · 2023-11-06T01:11:33Z

@agilgur5
I'm so sorry for the delay in responding😢
I was on a vacation and could not respond.
I should have left a message here beforehand.

I'm fixing the code you pointed out from today.

agilgur5 · 2023-11-06T01:48:13Z

No worries! I took a while to review while I was out sick too. A message or setting your GH status would be helpful, but no big deal either way.

Since you're new to OSS, for edification, things typically take longer in open-source as everyone is asynchronous and often has other higher priorities to attend to (like work, family, etc). So a few days or weeks lull is not that unexpected, in my experience, and I think is a healthier expectation given that many/most folks are volunteering time.

This is also a refactor and not blocking anything, so it doesn't have high urgency. If you can get it done sooner, great! If not, no problem, but a date may be helpful if someone wants to complete it in the interim.

Last but most importantly, hope you had a nice vacation! 🙂 All the better if you could turn your devices offline!

…ass-to-functional

Signed-off-by: tetora1053 <tetora1053@gmail.com>

juliev0 · 2023-11-23T01:11:29Z

Hi @tetora1053 @agilgur5 - I'm going through old PRs - is this one ready now for @agilgur5 to take another look at when he gets a chance? (and thank you, Anton!)

agilgur5 · 2023-11-23T01:42:02Z

is this one ready now for @agilgur5 to take another look at when he gets a chance? (and thank you, Anton!)

it is -- past 2 weeks have been a bit hectic in my personal life (as I've mentioned), so I'm behind a solid bit as a result

juliev0 · 2023-11-23T01:53:26Z

is this one ready now for @agilgur5 to take another look at when he gets a chance? (and thank you, Anton!)

it is -- past 2 weeks have been a bit hectic in my personal life (as I've mentioned), so I'm behind a solid bit as a result

I understand! Plus you're doing a lot. Just wanted to make sure it wasn't forgotten, but I don't think you lose sight of these.

@tetora1053 just FYI that we need more active approvers on the project, and so I'm trying to gauge interest in something like this: #12229

tetora1053 added 3 commits September 30, 2023 22:28

refactor(ui): WorkflowDag component from class to functional

746723c

Signed-off-by: tetora1053 <tetora1053@gmail.com>

fix: lint

1c2046e

Signed-off-by: tetora1053 <tetora1053@gmail.com>

fix: lint

1b09f17

Signed-off-by: tetora1053 <tetora1053@gmail.com>

tetora1053 marked this pull request as ready for review September 30, 2023 16:14

tetora1053 commented Sep 30, 2023

View reviewed changes

agilgur5 self-requested a review September 30, 2023 16:33

tetora1053 commented Sep 30, 2023

View reviewed changes

agilgur5 added the area/ui label Sep 30, 2023

agilgur5 mentioned this pull request Oct 13, 2023

UI: Refactor to use functional components #9810

Open

53 tasks

agilgur5 reviewed Oct 13, 2023

View reviewed changes

tetora1053 and others added 5 commits November 7, 2023 09:26

Merge branch 'master' into refactor-ui-workflow-dag-component-from-cl…

bef75a7

…ass-to-functional

refactor: memoize graph

d50f4bd

Signed-off-by: tetora1053 <tetora1053@gmail.com>

fix: lint

97835b0

Signed-off-by: tetora1053 <tetora1053@gmail.com>

fix: give the type when declaring expandNodes variable

a842280

Signed-off-by: tetora1053 <tetora1053@gmail.com>

fix: use defined func for consistency

5bb1061

Signed-off-by: tetora1053 <tetora1053@gmail.com>

agilgur5 mentioned this pull request Nov 9, 2023

steps not displaying in UI after v3.5.1 #12165

Closed

3 tasks

tetora1053 requested a review from agilgur5 November 11, 2023 08:07

agilgur5 self-assigned this Dec 28, 2023

agilgur5 mentioned this pull request Feb 1, 2024

REQUEST: Promotion to Approver for @agilgur5 argoproj/argoproj#277

Closed

7 tasks

agilgur5 added the type/tech-debt label Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ui): WorkflowDag component from class to functional #11920

refactor(ui): WorkflowDag component from class to functional #11920

tetora1053 commented Sep 30, 2023 •

edited

Loading

tetora1053 Sep 30, 2023 •

edited

Loading

tetora1053 Sep 30, 2023

tetora1053 Sep 30, 2023

agilgur5 left a comment •

edited

Loading

agilgur5 Oct 13, 2023

agilgur5 Oct 13, 2023

tetora1053 Nov 8, 2023

agilgur5 Oct 13, 2023

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 Oct 13, 2023

agilgur5 Oct 13, 2023

agilgur5 Oct 13, 2023 •

edited

Loading

tetora1053 Nov 7, 2023

tetora1053 Nov 7, 2023

tetora1053 Nov 8, 2023

agilgur5 commented Oct 28, 2023

tetora1053 commented Nov 6, 2023

agilgur5 commented Nov 6, 2023 •

edited

Loading

juliev0 commented Nov 23, 2023

agilgur5 commented Nov 23, 2023

juliev0 commented Nov 23, 2023

	const node = props.nodes[item.nodeName];
	const node = getNode(item.nodeName);

	const {edges, nodes} = (graph = new Graph());
	graph = new Graph();
	const {edges, nodes} = graph;

refactor(ui): WorkflowDag component from class to functional #11920

Are you sure you want to change the base?

refactor(ui): WorkflowDag component from class to functional #11920

Conversation

tetora1053 commented Sep 30, 2023 • edited Loading

Motivation

Modifications

Verification

Reference PRs

tetora1053 Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

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

agilgur5 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Oct 13, 2023 • edited Loading

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

agilgur5 commented Oct 28, 2023

tetora1053 commented Nov 6, 2023

agilgur5 commented Nov 6, 2023 • edited Loading

juliev0 commented Nov 23, 2023

agilgur5 commented Nov 23, 2023

juliev0 commented Nov 23, 2023

tetora1053 commented Sep 30, 2023 •

edited

Loading

tetora1053 Sep 30, 2023 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 Oct 13, 2023 •

edited

Loading

agilgur5 commented Nov 6, 2023 •

edited

Loading