-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
refactor(ui): convert WorkflowsList
+ WorkflowsFilter
to functional components
#11891
refactor(ui): convert WorkflowsList
+ WorkflowsFilter
to functional components
#11891
Conversation
- `this.props` -> `props` - methods -> inner functions - and some of these methods were static, so just moved them out of the component and into module-level - refactor `labelSuggestion` and `getPhaseItems` to two memoized variables instead - neither of them need to be getters / methods, they just compute a value that is used once in the component - there's a bit of computation in them too, so might as well memoize them - this is basically a classic use-case for `useMemo` Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- this is one of the most complex components in the UI, if not the most complex - `this.state` -> `useState` - methods -> inner functions - and some of these methods were static, so just moved them out of the component and into module-level - reorder various functions as inner functions must be ordered by usage, whereas methods do not - `componentDidMount` -> `useEffect` - `componentWillUnmount` -> `useEffect` disposer - `BasePage` + `this.queryParams` -> `location` argument + `URLSearchParams` - remove deprecated `BasePage` component entirely - this was the last reference to it, see my [previous](6fbfedf) [commits](c6fdb03) that removed the other usage of it - removes deprecated private `context` before React had an official Context API (yea, it's pretty old) - uses newer (but not necessarily newest) react-router APIs, which should unblock upgrading react-router as well - replace `Consumer` HoC with `useContext` hook - remove deprecated and unused `Query` component entirely - stumbled upon it searching for `Consumer` usage (there are a few left, mostly in process of being refactored in other PRs) - `.collectEvent` -> `useCollectEvent` - convert private variable `listWatch` to `useRef` - and modify usage to have `.current` - also use newer optional chaining syntax to simplify some lines - promise chains -> newer async/await syntax - consts assigned to anonymous functions -> named functions - modify `historyUrl` to also be able to take a `URLSearchParams` object - it didn't quite handle multiple values per key, so this is a bit of workaround for that - this function is doing a bit too much (including templating that is native in ES6+), so I think it should be refactored, but I first consolidated all remaining class-based code (in other refactor commits) to use it - now that all usage is consistent at least, it may be easier to refactor - consolidate functions that are used once in `saveHistory` to just run directly in `saveHistory` - which helps a bit with all the variable passing and shadowing (see below) NOTE: this commit on its own does not pass lint -- there's a lot of shadowing due to the renamings - these will be modified to use effects in the next commit, which should resolve the shadowing as well Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- no need to check `.has` as `.set` will overwrite - fixed a subtle bug -- `currentlySelected` was assigned to `selectedWorkflows` - this is a reference, so this ended up directly modifying a rendered variable - refactored this to just start from scratch as it goes through all selected anyway - also happens to simplify the logic a good bit too Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- same as previous commit, it was doing an in-place update - need to clone it first so that it's immutable - see also `immer` and `use-immer` - also rename `subWf` to just `wf` - it's not a sub-workflow? not sure why it was named that way Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- labels were reset to none when one was supposed to be removed, which was not quite correct behavior - it should just add or remove the selected label - also rename to `newLabel` instead of `newTag`, which was unnecessarily different and confusing Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- rewrite `fetchWorkflows` (and `saveHistory`) as an effect instead of a callback - similar to the other parts of the codebase - it's also responsible for synchornizing state with the API, matching [an explicit use-case](https://react.dev/reference/react/useEffect#connecting-to-an-external-system) of effects - this allows for some pretty significant optimizations - `fetchWorkflows` does not need to set a bunch of state variables -- once those state variables are updated, they will trigger a new fetch - so just set the newly changed variables and the state will be synchronized automatically by the effect - there are some nuances to the effect dependencies though, as it uses [referential equality](https://react.dev/reference/react/useEffect#parameters), so convert some objects and arrays to primitives - this was causing a really hard to figure out bug as well as some more subtle ones - modify several other pages that I checked to also use primitives for their effect dependencies - this might be an optimization and a subtle fix - `changeFilters` is no longer needed as a wrapper function since fetching is automatic and does less setting now - also remove an unnecessary `changeFilters` call in the `WorkflowToolbar`'s `load` function - if any batch actions are performed, the `ListWatch` will automatically pick them up, so there is no need to refetch - tested and made sure of this too - with `listWatch` usage consolidated into one effect, `useRef` is no longer needed - `saveHistory` no longer has to be in a callback, and can have a more optimized effect for itself as well - also rename variables to be shorter and match other files - `selectedPhases` -> `phases` and similar - make localStorage backward compatible with the new names so that old preferences continue working - there are some further optimizations that I think can be made to batch actions and selectedWorkflows - pretty sure the batch actions list can just be derived and memoized instead of being its own state variable - selectedWorkflows may not need as many resets or potentially could be consolidated - gonna do this in next commits - date filter and pagination filter should also be derived on Workflows - instead of acting at fetch time - the date filter in particular is entirely client-side, so should not need a server interaction to run Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- no need for state for this - also simplify some naming from `currentlySelected` to `newSelections` - more conventional as well as aligned with other parts of the codebase and this file - this one was a bit of an odd one out Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- same as in other functional components Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- if only date filters change, this shouldn't cause or need a refetch - same if the number of retrieved workflows is beyond the pagination limit - we can just not render them instead, saving a whole network trip - can confirm that date filter changes no longer cause a new request - this did require a bit of refactoring to use `filteredWorkflows` instead of `workflows` for a few comparisons - also refactor out a `clearSelectedWorkflows` function that's used in several places - so as to not repeat all the map creations etc - which is less error prone as it's a bit tedious to type out and as a result easy to mistype - fix a random typing issue that popped up -- `workflow.status.phase` is a `WorkflowPhase`, not a `NodePhase` - all existing type checking seems to have passed Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- it wasn't resetting after I made the referential equality changes - I had to stare at it for a while to realize why -- the `tags` state was initialized with props, but wasn't updated when props change - notably this was the past behavior before I refactored TagsInput to use hooks, but maybe didn't pop up if they were always equal - versus with the effect, if they're equal, it's not running `onChange` at all - so using an effect here is a bit of anti-pattern, but I matched the other parts of the codebase, but there is no need for state for `tags` in the first place - just remove it entirely and use `props` and now there is no duplication possibilities - also `onChange` is _always_ used by all components that use `TagsInput`, so that doesn't need to be optional - and if it's not optional, props tags and state tags should all be equivalent - maybe at one point this component was used with some internal state and an initial list, but not anymore Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- now that state is individual, this is much more natural and is mostly clean pass-through - any change func previously only affected one variable anyway, now it is explicit and less confusing (I definitely made some mistakes with that before) - this should be more efficient too, as it doesn't attempt to change all variables now and doesn't rely on equality checks for that to be efficient Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- b/c [old code](https://github.com/argoproj/argo-workflows/blob/c86a5cdb1ec1155e6ed17e67b46d5df59a566b08/ui/src/app/workflows/components/workflows-list/workflows-list.tsx#L108) relies on these existing, this glitched the heck out of me hard when I switched to a branch without this change - when it looked up the localStorage, it got `undefined` for phases and labels instead of an empty array, which caused a run-time error in the UI - I tried figuring out what was going on for a while, thinking I had a bug somewhere or in previous code (even tried reverting commits etc), but it was actually due to localStroage - clearing localStorage fixed the problem as the old code would then get defaults - as there could be a very real scenario where a user upgrades and then downgrades back (e.g. just testing an RC), wouldn't want to absolutely break their UI as a result - and idk how they'd know to clear their localStorage without asking around or trial-and-error -- the error looks very much like a bug - so make it so they won't experience this issue if they do upgrade and downgrade - it's actually not backward-compatible now but forward-compatible with an old version now -- as the comment says - also complete remaining rename of `selectedPhases` + `selectedLabels` -> `phases` + `labels` Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
I've left a comment, but everything work fine.
@@ -5,20 +5,24 @@ import {Utils} from './utils'; | |||
* Return a URL suitable to use with `history.push(..)`. Optionally saving the "namespace" parameter as the current namespace. | |||
* Only "truthy" values are put into the query parameters. I.e. "falsey" values include null, undefined, false, "", 0. | |||
*/ | |||
export const historyUrl = (path: string, params: {[key: string]: any}) => { | |||
export function historyUrl(path: string, params: {[key: string]: any}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we have unit tests for extraSearchParams
in ui/src/app/shared/history.test.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point! I forgot it had unit tests.
That being said I do think this function should be refactored a bit, so it may not be worth adding more tests at this time for something that will change. I intended extraSearchParams
as more a hacky interim workaround.
Should definitely revamp the tests when this gets refactored!
Partial fix for #9810
Partial fix for #11740
Motivation
BasePage
should unblock upgradingreact-router
BasePage
was old enough that it used the private context API before React had an official context API (the official context API is ~5.5 years old and this API is even older 😬 )Reports
to functional component and split files #11794 and refactor(ui): migrateUserInfo
to functional component #11793 which removed the other remaining uses of itArray.includes
syntaxesModifications
Changes are similar in structure to #11794, in case that helps review
Removals
BasePage
BasePage
+this.queryParams
->location
prop +URLSearchParams
Query
component entirelyConsumer
usage (there are a few left, mostly in process of being refactored in other PRs)Bugfixes
More Subtle Bugs
There were a few subtle bugs due to accidental in-place changes to Maps
immer
that are commonly used for this), so an object must be cloned first before one can make changesI stumbled upon some subtle bugs due to
useEffect
dependencies having objects, which can fail the referential equality checktoString()
on a string array, or using individual primitives in an object)workflow
), but I didn't touch those more complicated ones (at least for now)TagsInput
had a bug on a bug. One from previous code, and one was a reference equality check I screwed up in a refactor, that had the underlying bugprops.tags
was shadowed with statefultags
, which was not necessary and caused complications as well as made it easy to make bugs like theseprops.onChange
doesn't have to be optional as all uses of the component set it(Also were several cases of shadowing mid-refactor, but those are not visible in the PR diff)
Optimizations
Some state, like
batchActionDisabled
, could actually be entirely derived, so do that instead of usingstate
useMemo
on top of that tooDo all client-side filtering without requiring a refetch
filteredWorkflows
instead ofworkflows
for a few comparisonsListWatch
logic a good bit, which made it easier to convert to an effect (see below)Effects
rewrite
fetchWorkflows
as an effect instead of a callback, similar to previous refactors and other parts of the codebasefetchWorkflows
does not need to set a bunch of state variables -- once those state variables are updated, they will trigger a new fetchchangeFilters
is no longer needed as a wrapper function since fetching is automatic and does less setting nowchangeFilters
call in theWorkflowToolbar
'sload
functionListWatch
will automatically pick them up, so there is no need to refetchsaveHistory
no longer has to be in a callback, and can have a more optimized effect for itself as wellconvert
saveHistory
to an effect, similar to previous refactors and other parts of the codebaselocalStorage
saves as wellhistoryUrl
helper function to also be able to take aURLSearchParams
objectSimplifications
Refactor
WorkflowFilters
props andonChange
function to individual change functionsonChange
function was a catch-all but only ever used to change one variable at a timeRefactor out several
setSelectedWorkflows(new Map<string, Workflow>())
to aclearSelectedWorkflows
functionconsolidate functions that are used once in
saveHistory
to just in-line directly insaveHistory
Renaming Variables
selectedPhases
+selectedLabels
to justphases
andlabels
CronWorkflowsList
andReports
(that follow a similar code structure too)currentlySelected
tonewSelections
Compatibility changes
localStorage
, some tiny backward-compat code was needed, see in-line commentslocalStorage
causing the bug -- or rather the previous code's expectations thatlocalStorage
would return all variables and not possibly undefinedOther Fairly Straightforward Changes
Similar to previous refactor PRs:
this.props
->props
argumentthis.state
->useState
constructor
-> initial statemethods, getters, setters -> inner functions
render
functions to just directly in the main renderprivate variables, some getters -> inner variables, sometimes memoized
componentDidMount
->useEffect
componentWillUnmount
->useEffect
disposerreplace
Consumer
HoC withuseContext
hook.collectEvent
->useCollectEvent
promise chains -> newer async/await syntax
consts assigned to anonymous functions -> named functions
fix a random typing issue that popped up --
workflow.status.phase
is aWorkflowPhase
, not aNodePhase
Verification
Tested locally quite extensively and repeatedly. Tested batch actions and selections. Tested phase, label, template, cron, and date filters. Tested watch functionality.
Misc Notes
This was an absolute monster of a PR to write (see also the 13 commits over several days) and is the most complex component in the codebase as far as I can tell.
Getting ~600+ lines of very stateful and effectful code to be semantically equivalent was a challenge in and of itself, but had that working on like 3rd try. It did make my head hurt to figure out how to rewrite everything equivalently.
Then all the refactors, simplifications, optimizations and bugfixes came (some of which found or introduced new bugs that needed fix), which also made my head hurt after I kept finding more and more things to fix.
The new state of things is a good bit cleaner and reads fairly straightforwardly though (the past version took a bit to wrap your head around as there were some unexpected code paths). Hopefully this makes it much easier to modify in the future and for new contributors to get familiar with at least 😅
I'm also totally gonna take a break from front-end for a bit after this monstrosity and focus more on CI/build and back-end