refactor(ui): workflow panel components from class to functional #11803

tetora1053 · 2023-09-12T01:52:59Z

Partial fix for #9810
Partial fix for #11740

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
anonymous functions -> named functions
chaining promise callbacks -> async/await & try/catch
useMemo for optimization
hoist up static variable out of component

Verification

Mostly no real semantic changes
manually tested in local environment
submit panel

resubmit panel

retry panel

References PR

refactor(ui): convert a few components to use hooks #11800

tetora1053 · 2023-09-12T04:13:19Z

Sorry, I added a commit.
Removed a state that is set but not used at SubmitWorkflowPanel.
I have confirmed that no degradation has occurred in the local environment.

toyamagu-2021

Added comment, but I confirmed no regressions.

toyamagu-2021 · 2023-09-12T05:25:49Z

ui/src/app/workflows/components/submit-workflow-panel.tsx

+    const [entrypoint, setEntrypoint] = useState<string>(workflowEntrypoint);
+    const [parameters, setParameters] = useState<Parameter[]>([]);
+    const [workflowParameters, setWorkflowParameters] = useState<Parameter[]>(JSON.parse(JSON.stringify(props.workflowParameters)));
+    const [templates] = useState<Template[]>([defaultTemplate].concat(props.templates));


Originally state is used, but should we change here without useState?

const templates = [defaultTemplate].concat(props.templates);

Yea this doesn't seem like it actually uses state, just derives.
If that was meant to be a performance optimization, useMemo should be used instead (I believe there's a few places in the code where it seemed like it might be more appropriate for useMemo).

I left out most performance optimizations in my refactor PRs

I changed templates from state to const and memoized the select options created based on templates
Could you review if the changes are appropriate?
9050132

I think the memoization might not work as you're comparing templates, which is an array. useMemo uses Object.is to check equality of the dependencies, which uses reference equality for objects (and arrays). So it will probably get recalculated every time as templates is recalculated it above it every time (and therefore has a new reference each time).

I'm fine with no useMemo and just having it be a normal variable, as the performance difference is not really significant. Otherwise can try using props.templates as the dependency instead (as templates is wholly derived from props.templates, they are semantically equivalent dependencies), which I think would pass reference equality checks as it does not change after the initial fetch of the WorkflowTemplate. Along the same lines, if you use useMemo on the templates variable as well, that should also suffice for reference equality (as templates's reference would no longer change then)

thanks for detailed explanation on useMemo!
I now have a better understanding. (I didn't understand it before😃)
I fixed it and confirmed that variables templates and templatesOptions are not recalculated using console.log.

Glad to help 🙂
New version looks correct to me!

agilgur5

mostly looks good to me, though I've requested some further improvements in-line. basically to use named functions, async/await, optional chaining, type inference, and some small optimizations

agilgur5 · 2023-09-12T19:34:04Z

ui/src/app/workflows/components/resubmit-workflow-panel.tsx

-            memoized: false,
-            isSubmitting: false,
-            overrideParameters: false
+    const submit = () => {


Suggested change

const submit = () => {

function submit() {

prefer named function over const assigned to anonymous function (it's more performant and easier to trace in debug logs etc since "anonymous" is not the name -- I also used this in all of my recent PRs)

agilgur5 · 2023-09-12T19:34:39Z

ui/src/app/workflows/components/resubmit-workflow-panel.tsx

+                .then(handleSuccess)
+                .catch(handleError);
+        }
+    };


Suggested change

};

}

once it's a named func, the semi-colon will be auto removed by lint

agilgur5 · 2023-09-12T19:35:45Z

ui/src/app/workflows/components/resubmit-workflow-panel.tsx

+        const handleSuccess = (submitted: Workflow) => {
+            document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);
+        };


Suggested change

const handleSuccess = (submitted: Workflow) => {

document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);

};

function handleSuccess(submitted: Workflow) {

document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);

}

same as above.

or this gets removed entirely if submit is rewritten as it gets in-lined

agilgur5 · 2023-09-12T19:36:37Z

ui/src/app/workflows/components/resubmit-workflow-panel.tsx

-                        </div>
-                    </div>
+        const handleSuccess = (submitted: Workflow) => {
+            document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);


this did not change, but shouldn't this use react-router navigation instead?

Ah I think the current pattern is to use the History API with history.push. We are on an older version of react-router though (I'm working on upgrading it -- have to finish removing BasePage first), so I don't think the useHistory hook is available. Other components use history from props currently

thanks! I was just researching how to write in current react router version.
I'll try!

I tryed passing history in props and replaced 'document.location.href' with 'history.push', but it didn't work as I expected :(

props.history.push(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);

If the scope of the changes is likely to be larger than initially expected, it might be good to reconsider the approach after your react-router version up I think.

Does it eventually update itself to in progress?

no, the icon does not change at all from this image

before the modification, the icon had changed yellow -> blue -> green

Huh. Since this is on the same page (just different Workflow), I wonder if maybe the page is not quite unloading correctly and still has the old state. The effects seem to have proper disposers and dependencies, so I don't see anything strange off the top of my head.

At this point, especially since it potentially involves a whole other page's logic, I think we should punt on the navigation improvements for now. Appreciate your efforts and investigation here! Hopefully that will help the next person to tackle this too (which could be yourself 😉 ).

In the retry component, the state appears to be changing as expected, so the difference in implementation between retry and resubmit may be a clue, though I haven't been able to found out.

If I figure it out, I'll make a PR🔥

so the difference in implementation between retry and resubmit may be a clue

So a retry does not create a new Workflow, it just reruns failed steps. A resubmit creates a wholly new Workflow, effectively a clone of the old one.

So for a retry, the URL does not change. For a resubmit it does.
For the retry, the existing ListWatch might be able to pick up the change automatically. For a resubmit, an entirely new request is needed for the new Workflow.

Maybe those details may help you debug?
I already knew that though (I wrote a bit of the documentation on retry vs resubmit in #11625) and couldn't figure it out, though I didn't stare too hard

agilgur5 · 2023-09-12T19:37:07Z

ui/src/app/workflows/components/resubmit-workflow-panel.tsx

+        const handleError = (err: Error) => {
+            setError(err);
+            setIsSubmitting(false);
+        };


Suggested change

const handleError = (err: Error) => {

setError(err);

setIsSubmitting(false);

};

function handleError(err: Error) {

setError(err);

setIsSubmitting(false);

}

same as above.

or this gets removed entirely if submit is rewritten as it gets in-lined

I reverted handleSuccess and handleError funcs to inline because there was no need to extract functions by using await/async and try/catch as you pointed out. thanks!

agilgur5 · 2023-09-12T19:54:55Z

ui/src/app/workflows/components/submit-workflow-panel.tsx

+    const getSelectedTemplate = (name: string): Template | null => {
+        return templates.find(t => t.name === name) || null;
+    };


Suggested change

const getSelectedTemplate = (name: string): Template | null => {

return templates.find(t => t.name === name) || null;

};

function getSelectedTemplate(name: string): Template | null {

return templates.find(t => t.name === name) || null;

}

same as above re: named functions over anonymous

agilgur5 · 2023-09-12T19:56:26Z

ui/src/app/workflows/components/submit-workflow-panel.tsx

+                        onChange={selected => {
+                            const selectedTemp = getSelectedTemplate(selected.value);
+                            setEntrypoint(selected.value);
+                            setParameters((selectedTemp && selectedTemp.inputs.parameters) || []);


Suggested change

setParameters((selectedTemp && selectedTemp.inputs.parameters) || []);

setParameters(selectedTemp?.inputs?.parameters || []);

can simplify this with optional chaining syntax

ui/src/app/workflows/components/submit-workflow-panel.tsx

agilgur5 · 2023-09-12T19:57:34Z

ui/src/app/workflows/components/submit-workflow-panel.tsx

+    const submit = () => {
+        setIsSubmitting(true);
        services.workflows
-            .submit(this.props.kind, this.props.name, this.props.namespace, {
-                entryPoint: this.state.entrypoint === workflowEntrypoint ? null : this.state.entrypoint,
+            .submit(props.kind, props.name, props.namespace, {
+                entryPoint: entrypoint === workflowEntrypoint ? null : entrypoint,
                parameters: [
-                    ...this.state.workflowParameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p)),
-                    ...this.state.parameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p))
+                    ...workflowParameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p)),
+                    ...parameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p))
                ],
-                labels: this.state.labels.join(',')
+                labels: labels.join(',')
            })
            .then((submitted: Workflow) => (document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`)))
-            .catch(error => this.setState({error, isSubmitting: false}));
-    }
+            .catch(err => {
+                setError(err);
+                setIsSubmitting(false);
+            });
+    };


similar to above, could we convert this to async/await and a named function as well?

agilgur5 · 2023-09-12T19:58:18Z

ui/src/app/workflows/components/submit-workflow-panel.tsx

                ],
-                labels: this.state.labels.join(',')
+                labels: labels.join(',')
            })
            .then((submitted: Workflow) => (document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`)))


this hasn't changed, but also should perhaps use react-router navigation instead?

agilgur5 · 2023-09-12T20:18:59Z

Most of the PR description verbatim copy+pastes my previous PRs (e.g. #11800). That's fine, but please cite your sources, especially when it is verbatim.

agilgur5 · 2023-09-12T20:20:00Z

Would either of you like to review the other PRs I have open for the refactor? Could help speed up getting them merged 🙂

agilgur5 · 2023-09-14T01:51:50Z

Btw, the unrelated unit test failure was fixed in #11816. may want to rebase on top (or merge)

Signed-off-by: tetora1053 <[email protected]>

agilgur5

Since we're punting on navigation improvements for now, the one remaining thing is the memoization fixes

Signed-off-by: tetora1053 <[email protected]>

agilgur5

LGTM. Thanks for getting the refactors, iteration, and performance improvements done!

tetora1053 · 2023-09-16T00:04:57Z

@agilgur5
I want to express my gratitude.
this was my first contribution to OSS, and your constructive feedback and guidance made the process much smoother.
thank you for taking much time to review my pull request and for your invaluable advice.

having my pull request merged means a lot to me.
I'm looking forward to contributing more in the future!

and thank you a lot @toyamagu-2021 for passing this issue to me!

agilgur5 · 2023-09-19T02:15:28Z

Thanks for your heartfelt words @tetora1053! ❤️ Experiences like these are one of the reasons I continue to work in OSS 🙂

Glad I could help make it a great experience for you and hope to see more of your contributions! Big congrats on your first contribution 🎉 🎉

Thanks @toyamagu-2021 for getting more folks involved as well!

agilgur5 self-requested a review September 12, 2023 02:19

agilgur5 added the area/ui label Sep 12, 2023

tetora1053 marked this pull request as ready for review September 12, 2023 02:37

toyamagu-2021 approved these changes Sep 12, 2023

View reviewed changes

agilgur5 reviewed Sep 12, 2023

View reviewed changes

agilgur5 mentioned this pull request Sep 14, 2023

UI: Refactor to use functional components #9810

Open

53 tasks

tetora1053 added 9 commits September 15, 2023 02:20

refactor(ui): workflow panel components from class to functional

67abb6a

Signed-off-by: tetora1053 <[email protected]>

fix: remove a state just set but not used

9db7933

Signed-off-by: tetora1053 <[email protected]>

refactor: from anonymous function to named function

3829568

Signed-off-by: tetora1053 <[email protected]>

refactor: from anonymous function to named function

6737c3b

Signed-off-by: tetora1053 <[email protected]>

refactor: remove type annotations for primitives

9e1fd46

Signed-off-by: tetora1053 <[email protected]>

refactor: use async/await instead of chaining promise callbacks

48e214d

Signed-off-by: tetora1053 <[email protected]>

refactor: use optional chanining instead of &&

eb59c26

Signed-off-by: tetora1053 <[email protected]>

refactor: memoize templateOptions in entrypoint select options

6634c0a

Signed-off-by: tetora1053 <[email protected]>

refactor: hoist up static variable out of component

0e47525

Signed-off-by: tetora1053 <[email protected]>

tetora1053 force-pushed the refactor-ui-workflow-panel-components-functional branch from e370c67 to 0e47525 Compare September 14, 2023 17:20

agilgur5 reviewed Sep 14, 2023

View reviewed changes

fix: variables memoization

106027a

Signed-off-by: tetora1053 <[email protected]>

agilgur5 approved these changes Sep 15, 2023

View reviewed changes

agilgur5 requested a review from terrytangyuan September 15, 2023 18:15

terrytangyuan approved these changes Sep 15, 2023

View reviewed changes

terrytangyuan merged commit 92a30f2 into argoproj:master Sep 15, 2023
22 checks passed

tetora1053 deleted the refactor-ui-workflow-panel-components-functional branch September 15, 2023 23:56

agilgur5 mentioned this pull request Sep 22, 2023

REQUEST: Promotion to Reviewer for @agilgur5 argoproj/argoproj#242

Closed

8 tasks

This was referenced Sep 28, 2023

refactor(ui): InputFilter and WorkflowTimeline components from class to functional #11899

Merged

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

Open

agilgur5 mentioned this pull request Feb 1, 2024

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

Closed

7 tasks

	setParameters((selectedTemp && selectedTemp.inputs.parameters) \|\| []);
	setParameters(selectedTemp?.inputs?.parameters \|\| []);

refactor(ui): workflow panel components from class to functional #11803

refactor(ui): workflow panel components from class to functional #11803

Conversation

tetora1053 commented Sep 12, 2023 • edited Loading

Motivation

Modifications

Verification

References PR

tetora1053 commented Sep 12, 2023

toyamagu-2021 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

tetora1053 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

tetora1053 Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Sep 12, 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

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

agilgur5 Sep 12, 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 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 commented Sep 12, 2023 • edited Loading

agilgur5 commented Sep 12, 2023

agilgur5 commented Sep 14, 2023 • edited Loading

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

tetora1053 commented Sep 16, 2023 • edited Loading

agilgur5 commented Sep 19, 2023

tetora1053 commented Sep 12, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

tetora1053 Sep 13, 2023 •

edited

Loading

agilgur5 Sep 14, 2023 •

edited

Loading

tetora1053 Sep 15, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

agilgur5 Sep 12, 2023 •

edited

Loading

agilgur5 commented Sep 12, 2023 •

edited

Loading

agilgur5 commented Sep 14, 2023 •

edited

Loading

agilgur5 left a comment •

edited

Loading

tetora1053 commented Sep 16, 2023 •

edited

Loading