New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged

terrytangyuan merged 8 commits into argoproj:master from tetora1053:refactor-ui-components-from-class-to-functional

Sep 30, 2023

Contributor

tetora1053 commented Sep 28, 2023 •

edited

Loading

Partial fix for #9810

15. ui/src/app/shared/components/input-filter.tsx:

44. ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx:

Motivation

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

Modifications

this.state -> useState
this.props -> props
useRef to manage references to DOM elements
methods -> inner functions

Verification

Mostly no real semantic changes
manually tested in local environment

InputFilter

WorkflowTimeline

Reference PRs

tetora1053 added 3 commits

September 27, 2023 22:32


          refactor(ui): InputFilter component from class to functional

3263cfa

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


          refactor(ui): WorkflowTimeline component from class to functional

7adb9e9

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


          fix: remove redundant type annotations

dd5b64d

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

tetora1053 commented

View reviewed changes

ui/src/app/shared/components/input-filter.tsx

-              interface InputState {
-                  value: string;
-                  localCache: string[];
-                  error?: Error;

Contributor Author

tetora1053 Sep 28, 2023

error state isn't used, so removed it.


          refactor: variable name

9b7b7c7

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

tetora1053 commented

View reviewed changes

ui/src/app/shared/components/input-filter.tsx

-              }
+              export function InputFilter(props: InputProps) {
+                  const [value, setValue] = useState(props.value);
+                  const [localCache, setLocalCache] = useState((localStorage.getItem(props.name + '_inputs') || '').split(',').filter(item => item !== ''));

Contributor Author

tetora1053 Sep 28, 2023

ts infers the type of localCache correctly, so I omitted type specification. (useState<string[]>)
but at the same time, I think it also looks good to specify the type for readability.
which should I choose 😟

Contributor

agilgur5 Sep 28, 2023

yea same as your previous PR, I think when the inference is correct and straightforward (string in this case), there's no need to annotate it.

I also have experience working in ML-family languages like OCaml, which are even stronger typed and where the compiler infers the types of the entire program (so there are no type annotations at all). On the other spectrum would be something like Java, which requires on type annotations on almost everything and feels very verbose as a result (despite having weaker guarantees than OCaml et al).

For TS, I prefer to have strict typing on, which we'll be able to do in this codebase once there is some progress on decoupling argo-ui (see argoproj/argo-ui#453). With strict typing, if a type can't be correctly inferred, the compiler will tell you (right now I believe it only has a warning).

Contributor Author

tetora1053 Sep 29, 2023

I see. unless it's a complex type, I'll try to omit the type annotations where TS correctly infers. thanks for detailed description.😄

and I had no idea about argo-ui! Thank you for letting me know. I'm really hopeful for the progress in decoupling. It sounds it could bring great improvements.

tetora1053 marked this pull request as ready for review

September 28, 2023 17:36

agilgur5 self-requested a review

September 28, 2023 18:15

agilgur5 reviewed

View reviewed changes

Contributor

agilgur5 left a comment •

edited

Loading

just a few changes needed to refs in the WorkflowTimeline component, otherwise the rest looks good 😄

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

-                          this.refreshSubscription.unsubscribe();
-                          this.refreshSubscription = null;
+                  useEffect(() => {
+                      ensureRunningWorkflowRefreshing(props.workflow);

Contributor

agilgur5 Sep 29, 2023

we can in-line this function now as it's only used here now

Contributor Author

tetora1053 Sep 29, 2023 •

edited

Loading

thanks!
I in-lined the function, and I noticedcompletedPhase variable is static, so I hoisted it up to the module level
https://github.com/argoproj/argo-workflows/pull/11899/files#diff-24ff460ef8551421b42892db8b66a3636c1e3797bceb65679d51fb1547aab43bR14

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

Comment on lines 22 to 23

		let resizeSubscription: Subscription;
		let refreshSubscription: Subscription;

Contributor

agilgur5 Sep 29, 2023

Suggested change

      
                let resizeSubscription: Subscription;
          
                let refreshSubscription: Subscription;
          
                const resizeSubscription = useRef<Subscription>(null);
          
                const refreshSubscription = useRef<Subscription>(null);

these actually should be refs as well so that they are persisted between renders.
Right now I think it only still works by coincidence, since all the functions have it in their scope, but the best practice (and least error prone) here would be to use refs

Contributor

agilgur5 Sep 29, 2023

Could you also move these to be together with containerRef as they were before? so that all refs are together

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

-                      this.state = {parentWidth: 0, now: moment()};
-                      this.ensureRunningWorkflowRefreshing(props.workflow);
-                  }
+                  const containerRef = useRef(null);

Contributor

agilgur5 Sep 29, 2023

Suggested change

      
                const containerRef = useRef(null);
          
                const containerRef = useRef<HTMLElement>(null);

this one does need a type annotation 😉
(since null can't be inferred to be an HTMLElement)

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

-                      this.updateWidth();
-                  }
+                  useEffect(() => {
+                      resizeSubscription = fromEvent(window, 'resize').subscribe(updateWidth);

Contributor

agilgur5 Sep 29, 2023

Suggested change

      
                    resizeSubscription = fromEvent(window, 'resize').subscribe(updateWidth);
          
                    resizeSubscription.current = fromEvent(window, 'resize').subscribe(updateWidth);

will need a .current when it's converted to a ref

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

Comment on lines 34 to 40

+                      return () => {
+                          if (resizeSubscription) {
+                              resizeSubscription.unsubscribe();
+                          }
+                          if (refreshSubscription) {
+                              refreshSubscription.unsubscribe();
+                          }

Contributor

agilgur5 Sep 29, 2023

Suggested change

      
                    return () => {
          
                        if (resizeSubscription) {
          
                            resizeSubscription.unsubscribe();
          
                        }
          
                        if (refreshSubscription) {
          
                            refreshSubscription.unsubscribe();
          
                        }
          
                    return () => {
          
                        resizeSubscription.current?.unsubscribe();
          
                        refreshSubscription.current?.unsubscribe();

can simplify these with optional chaining syntax

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

-                              return diff;
+                  function ensureRunningWorkflowRefreshing(workflow: models.Workflow) {
+                      const completedPhases = [models.NODE_PHASE.ERROR, models.NODE_PHASE.SUCCEEDED, models.NODE_PHASE.SKIPPED, models.NODE_PHASE.OMITTED, models.NODE_PHASE.FAILED];
+                      const isCompleted = workflow && workflow.status && completedPhases.indexOf(workflow.status.phase) > -1;

Contributor

agilgur5 Sep 29, 2023

Suggested change

      
                    const isCompleted = workflow && workflow.status && completedPhases.indexOf(workflow.status.phase) > -1;
          
                    const isCompleted = workflow && workflow.status && completedPhases.includes(workflow.status.phase);

can use the newer includes function now

ui/src/app/workflows/components/workflow-timeline/workflow-timeline.tsx Outdated

Comment on lines 57 to 63

+                      if (!refreshSubscription && !isCompleted) {
+                          refreshSubscription = interval(1000).subscribe(() => {
+                              setNow(moment());
                           });
-                      if (nodes.length === 0) {
-                          return <div />;
+                      } else if (refreshSubscription && isCompleted) {
+                          refreshSubscription.unsubscribe();
+                          refreshSubscription = null;

Contributor

agilgur5 Sep 29, 2023

Suggested change

      
                    if (!refreshSubscription && !isCompleted) {
          
                        refreshSubscription = interval(1000).subscribe(() => {
          
                            setNow(moment());
          
                        });
          
                    if (nodes.length === 0) {
          
                        return <div />;
          
                    } else if (refreshSubscription && isCompleted) {
          
                        refreshSubscription.unsubscribe();
          
                        refreshSubscription = null;
          
                    if (!refreshSubscription.current && !isCompleted) {
          
                        refreshSubscription.current = interval(1000).subscribe(() => {
          
                            setNow(moment());
          
                        });
          
                    } else if (refreshSubscription.current && isCompleted) {
          
                        refreshSubscription.current.unsubscribe();
          
                        refreshSubscription.current = null;

will need to add .current here when it's a ref as well

Contributor Author

tetora1053 Sep 29, 2023 •

edited

Loading

while making the modifications, I was wondering, is the line refreshSubscription.current = null; necessary?
wouldn't refreshSubscription.current.unsubscribe(); take care of it? 🤔

ui/src/app/shared/components/input-filter.tsx

-                      };
+                  function setValueAndCache(newValue: string) {
+                      setLocalCache(currentCache => {
+                          const updatedCache = [...currentCache];

Contributor

agilgur5 Sep 29, 2023

nice catch to make this an immutable operation!

tetora1053 and others added 2 commits

September 29, 2023 23:54


          fix: optimize code

f2f6973

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


          Merge branch 'master' into refactor-ui-components-from-class-to-funct…

debbd7b

…ional

agilgur5 approved these changes

View reviewed changes

Contributor

agilgur5 left a comment •

edited

Loading

LGTM. Thanks for updating this and fixing some subtle bugs (like the mutable updates) in the process too!

agilgur5 requested a review from terrytangyuan

September 30, 2023 01:59

terrytangyuan approved these changes

View reviewed changes

Member

terrytangyuan left a comment

Thanks!

agilgur5 added the area/ui label

terrytangyuan enabled auto-merge (squash)

September 30, 2023 02:00

terrytangyuan added 2 commits

September 29, 2023 22:00


          Merge branch 'master' into refactor-ui-components-from-class-to-funct…

25b644d

…ional


          Merge branch 'master' into refactor-ui-components-from-class-to-funct…

409c7ea

…ional

terrytangyuan merged commit 68ad039 into argoproj:master

25 checks passed

tetora1053 deleted the refactor-ui-components-from-class-to-functional branch

September 30, 2023 04:17

agilgur5 mentioned this pull request

UI: Refactor to use functional components #9810

Open

53 tasks

agilgur5 mentioned this pull request

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

Closed

7 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels