refactor(ui): migrate `Reports` to functional component and split files #11794

agilgur5 · 2023-09-10T20:23:40Z

Partial fix for #9810
Partial fix for #11740

Motivation

Use newer, modern functional components compatible with hooks instead of legacy class-based components
Use newer, modern async/await syntax instead of Promise chains
also remove a deprecated, upgrade-blocking usage of BasePage similar to refactor(ui): migrate UserInfo to functional component #11793

Modifications

Follow a very similar process to #7035

state -> useState
this.props -> arguments
BasePage + this.queryParams -> location argument + URLSearchParams
componentDidMount -> useEffect
services.info.collectEvent -> useCollectEvent
saveHistory -> useEffect + historyUrl
Consumer -> useContext
merge renderReport into the main component
split out filters into a separate ReportsFilters component similar to CronWorkflowFilters, WorkflowFilters, etc
- be consistent with how all these work
- similar to feat(ui): add label/state filter to cronworkflow. Fixes #7034 #7035, refactor logic to have an onChange function and retrieve Workflows in a top-level effect (after all state has been set)
- and similarly add an SCSS file with the appropriate styles
  - note: this may be worth refactoring as nearly all the filters have the same styles. comments in feat(ui): add label/state filter to cronworkflow. Fixes #7034 #7035 suggest making a shared filters component if possible too.
Promise chains -> async/await
- refactor ReportsFilters logic to use async/await syntax
- also refactor CronWorkflowList to use newer async/await syntax consistent with UI: Refactor to use async/await syntax #11740
getters and setters to regular functions
split out a pure function into a util file
- to make clear that it is pure and unrelated to React state, context, etc

Also some tiny optimizations similar to #11743:

use named functions instead of consts assigned to anonymous functions
- this makes tracing a bit easier and source maps a bit clearer
- (also technically a memory improvement as well)

Verification

Pure refactor, no real semantic changes
Test manually, seems to work, still very interactive with fancy animations
- It was my first time using this page though, I literally didn't know it existed 👀
  - I believe it's not really documented either?
- Screenshot below of it working:

- follow a very similar process to 25e1939 - `state` -> `useState` - `this.props` -> arguments - `BasePage` + `this.queryParams` -> `location` argument + `URLSearchParams` - `componentDidMount` -> `useEffect` - `services.info.collectEvent` -> `useCollectEvent` - `saveHistory` -> `useEffect` + `historyUrl` - `Consumer` -> `useContext` - merge `renderReport` into the main component - will move filters into a separate component in the next commit, following existing convention and the commit - Promise chains -> async/await - getters and setters to regular functions - split out a pure function - can move this into a util file in the next commit - also modify some comments in `cron-workflows-list` - convert variable assigned to anonymous function to a named function - better practice, better tracing, less memory, etc Signed-off-by: Anton Gilgur <[email protected]>

- split out pure function into its own file - to make clear that it is pure and unrelated to React state, context, etc - split out `ReportsFilters` component similar to `CronWorkflowFilters`, `WorkflowFilters`, etc - be consistent with how all these work - similar to 25e1939, refactor logic to have an `onChange` function and retrieve Workflows in a top-level effect (after all state has been set) - and similarly add an SCSS file with the appropriate styles - note: this may be worth refactoring as nearly all the filters have the same styles - refactor `ReportsFilters` logic to use async/await syntax - also refactor `CronWorkflowList` to use newer async/await syntax consistent with future state of codebase Signed-off-by: Anton Gilgur <[email protected]>

Signed-off-by: Anton Gilgur <[email protected]>

agilgur5

It's possible to optimize this a bit with useCallback. And I think most of the functional components are memoizable too.

But that can be done in a follow-up optimization PR if needed, leaving as is for now for least potential breakage. It also runs pretty fast locally anyway so nbd probably.

agilgur5 · 2023-09-10T22:19:15Z

ui/src/app/reports/components/reports.tsx

+        history.push(historyUrl('reports' + (Utils.managedNamespace ? '' : '/{namespace}'), {namespace, labels: labels.join(',')}));
+    }, [namespace, labels]);
+
+    async function onChange(newNamespace: string, newLabels: string[]) {


potential useCallback optimization

agilgur5 · 2023-09-10T22:19:36Z

ui/src/app/reports/components/reports-filters.tsx

+    function getLabel(name: string) {
+        return (labels.find(label => label.startsWith(name)) || '').replace(name + '=', '');
+    }
+
+    function setLabel(name: string, value: string) {
+        onChange(namespace, labels.filter(label => !label.startsWith(name)).concat(name + '=' + value));
+    }
+
+    function getPhase() {
+        return getLabel(labelKeyPhase);
+    }
+
+    function setPhase(value: string) {
+        setLabel(labelKeyPhase, value);
+    }
+
+    function setWorkflowTemplate(value: string) {
+        setLabel(labelKeyWorkflowTemplate, value);
+    }
+
+    function setCronWorkflow(value: string) {
+        setLabel(labelKeyCronWorkflow, value);
+    }


potential useCallback optimizations

toyamagu-2021

I've double chacked functionality.

ui/src/app/reports/components/reports-filters.tsx

isubasinghe

Looks good, no need to worry about useCallback for now imo.
Might even be slower for some functions, anyway I think the performance of the UI is not a worry for me regardless.

Oh the only exception being the swagger docs page which is unusable but yes not related to this issue.

agilgur5 · 2023-09-15T21:17:25Z

Looks good, no need to worry about useCallback for now imo. [...] anyway I think the performance of the UI is not a worry for me regardless.

Agreed, figure I'd notate it at least. I used to write fully memoized components, but I'd forgotten some of the optimizations like useCallback (since React hasn't been my day job in years). Some of those UIs I wrote in the past were impressively performant, so it can make a difference at scale (scale with respect to number of rerenders and components).

Oh the only exception being the swagger docs page which is unusable but yes not related to this issue.

Yea it freezes everytime I try to use it. I can't even run a profiler on it. It may very well be on the swagger-ui side though, or due to the size of the swagger. I have not seen this problem in other codebases though (but idr if we used different renderers or not).

ui/src/app/reports/components/reports.tsx

agilgur5 added 2 commits September 10, 2023 16:10

agilgur5 added the area/ui label Sep 10, 2023

agilgur5 mentioned this pull request Sep 10, 2023

refactor(ui): migrate UserInfo to functional component #11793

Merged

agilgur5 requested a review from terrytangyuan September 10, 2023 20:57

fix leftover this.state usage

1972369

Signed-off-by: Anton Gilgur <[email protected]>

agilgur5 mentioned this pull request Sep 10, 2023

UI: Refactor to use functional components #9810

Open

53 tasks

agilgur5 commented Sep 10, 2023

View reviewed changes

agilgur5 mentioned this pull request Sep 11, 2023

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

Merged

agilgur5 requested a review from toyamagu-2021 September 14, 2023 02:18

toyamagu-2021 approved these changes Sep 14, 2023

View reviewed changes

ui/src/app/reports/components/reports-filters.tsx Show resolved Hide resolved

isubasinghe approved these changes Sep 15, 2023

View reviewed changes

terrytangyuan approved these changes Sep 21, 2023

View reviewed changes

terrytangyuan merged commit c6fdb03 into argoproj:master Sep 21, 2023
24 checks passed

agilgur5 deleted the refactor-ui-reports-functional branch September 21, 2023 02:35

This was referenced Sep 22, 2023

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

Closed

refactor(ui): convert WorkflowsList + WorkflowsFilter to functional components #11891

Merged

agilgur5 mentioned this pull request Oct 21, 2023

UI: Code Split larger and/or less-used paths for smaller initial bundle #12059

Open

19 tasks

agilgur5 commented Oct 21, 2023

View reviewed changes

ui/src/app/reports/components/reports.tsx Show resolved Hide resolved

agilgur5 mentioned this pull request Oct 21, 2023

fix(ui): remove accidentally rendered semi-colon #12060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ui): migrate `Reports` to functional component and split files #11794

refactor(ui): migrate `Reports` to functional component and split files #11794

agilgur5 commented Sep 10, 2023 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Sep 10, 2023

agilgur5 Sep 10, 2023

toyamagu-2021 left a comment

isubasinghe left a comment •

edited

Loading

agilgur5 commented Sep 15, 2023

refactor(ui): migrate Reports to functional component and split files #11794

refactor(ui): migrate Reports to functional component and split files #11794

Conversation

agilgur5 commented Sep 10, 2023 • edited Loading

Motivation

Modifications

Verification

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 Sep 10, 2023

Choose a reason for hiding this comment

agilgur5 Sep 10, 2023

Choose a reason for hiding this comment

toyamagu-2021 left a comment

Choose a reason for hiding this comment

isubasinghe left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 commented Sep 15, 2023

refactor(ui): migrate `Reports` to functional component and split files #11794

refactor(ui): migrate `Reports` to functional component and split files #11794

agilgur5 commented Sep 10, 2023 •

edited

Loading

agilgur5 left a comment •

edited

Loading

isubasinghe left a comment •

edited

Loading