perf(redux): optimize state persist and add slice versions #753

tthvo · 2022-12-13T06:45:32Z

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Read the contributing guidelines
Linked a relevant issue which this PR resolves
Linked any other relevant issues, PR's, or documentation, if any
Resolved all conflicts, if any
Rebased your branch PR on top of the latest upstream main branch
Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
Signed the last commit: git commit --amend --signoff

Fixes: #725
Depends on #747 (needs updates on dashboardCondfigReducer)

Description of the change:

This PR includes following changes:

Organized redux reducers and actions into slices.
- Each slice now belongs to "kind" directory.
- Each slice consists of related actions and reducers.
- ReduxStore.tsx nows exports all actions so that we can just import from there. This helps if in the futures, this redux folder changes.
Avoid persisting calls to localStorage that saves entire redux store stage.

Motivation for the change:

As redux store grows, the number of reducers and actions increase. Therefore, we need a more organized folder structure.
Persisting states previously store the entire store for any state change. This should be fined-grained for each reducer.

tthvo · 2022-12-13T06:57:27Z

There was a little hiccup from redux-persist for tests:

_persist key is automatically added to state, but is marked as required (Make '_persist' key optional in state returned by persistReducer rt2zz/redux-persist#1170). Tests must add this with:
```
{
  _persist: {
    version: 1:
    rehydrated: false
  },
...
}
```

That PR has not been merged for long :((

github-actions · 2022-12-13T20:23:21Z

This PR/issue depends on:

~~feat(dashboard): card configuration #747~~
By Dependent Issues (🤖). Happy coding!

andrewazores · 2022-12-13T20:31:13Z

There was a little hiccup from redux-persist for tests:

* `_persist` key is automatically added to state, but is marked as required ([Make '_persist' key optional in state returned by persistReducer rt2zz/redux-persist#1170](https://github.com/rt2zz/redux-persist/pull/1170)). Tests must add this with:
  ```ts
  {
    _persist: {
      version: 1:
      rehydrated: false
    },
  ...
  }
  ```

That PR has not been merged for long :((

I'm a little concerned about this package given that that PR has been open so long, despite being so small and seemingly low risk. Development activity looks pretty dead in general. What if it stays dead and falls behind the latest Redux versions?

Maybe this is an opportunity to re-investigate usage of Redux as a whole, and find something comparable that also offers localStorage persistence?

https://github.com/pmndrs/zustand
https://docs.pmnd.rs/zustand/integrations/persisting-store-data

This looks like it operates from a very similar principle to Redux but probably even simpler, and comes with persistence support. Redux probably does better when you have large and complex global application state, but our application state is mostly on the backend actually and all the client state storage is (hopefully) small and simple UI state by design.

github-actions · 2022-12-13T20:33:06Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-2f05779acc1c56de7de7ad544bcfc65f734e392f sh smoketest.sh

tthvo · 2022-12-13T20:44:07Z

I'm a little concerned about this package given that that PR has been open so long, despite being so small and seemingly low risk. Development activity looks pretty dead in general. What if it stays dead and falls behind the latest Redux versions?

Maybe this is an opportunity to re-investigate usage of Redux as a whole, and find something comparable that also offers localStorage persistence?

https://github.com/pmndrs/zustand
https://docs.pmnd.rs/zustand/integrations/persisting-store-data

This looks like it operates from a very similar principle to Redux but probably even simpler, and comes with persistence support. Redux probably does better when you have large and complex global application state, but our application state is mostly on the backend actually and all the client state storage is (hopefully) small and simple UI state by design.

Right, there has not been any major dev since Oct 2021. I will look into this new package then. I guess we will move away from Redux then.

andrewazores · 2022-12-13T20:48:12Z

Well, not necessarily. We could stick to Redux since that's quite well established and active, just avoid redux-persist. If there is nothing else out there similar then maybe there are lessons we can take away and implement small pieces of as utilities within our own project at least to improve the local storage performance.

tthvo · 2022-12-13T20:53:30Z

Well, not necessarily. We could stick to Redux since that's quite well established and active, just avoid redux-persist. If there is nothing else out there similar then maybe there are lessons we can take away and implement small pieces of as utilities within our own project at least to improve the local storage performance.

I think apart from redux-persist, I was exploring writing a custom Redux middleware to persist data but stopped after seeing that package. I could try seeing if I could replicate current behaviour with middleware.

andrewazores · 2022-12-13T21:40:31Z

Frontend notifications could probably also be handled using Redux/Zustand/whatever, now that I think of it.

andrewazores · 2022-12-13T22:16:03Z

https://jotai.org/

This one looks good as well, from a cursory glance.

andrewazores · 2022-12-13T22:20:32Z

pmndrs/jotai#13

tthvo · 2022-12-13T23:19:00Z

@andrewazores Hey, with this latest commits, I tried to write a simple middleware that persists part of the store based on the action. Any new reducer will add another if in this middleware to persist state.

https://github.com/cryostatio/cryostat-web/pull/753/files#diff-84879185aeec16f8b14d20bb744308faa59665dc3485dbe6eb674a935da3af3fR46

What do you think? Meanwhile I will play around with other packages in case we want to migrate our redux setup to it.

github-actions · 2022-12-14T20:37:14Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-a1d32ca9b633cb108bcaa407a6b746eabb34b461 sh smoketest.sh

src/app/Shared/Redux/Middlewares/PersistMiddleware.tsx

tthvo · 2022-12-14T20:46:23Z

Ready for review again... :D

In these latest commits:

Resolve circular dependencies issue by moving all required "empty" state to redux slice files so that they are always initialized. Though, there actually still circular deps on interfaces but exported const are now resolved. We might need to be careful with future imports lint.
Renamed action intent (i.e. action generator) to have prefix that matchs specific slice kind.

github-actions · 2022-12-14T20:48:24Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-050d29bf4bf34ebf2d440f76873e95d6caa78ee1 sh smoketest.sh

github-actions · 2022-12-14T20:57:12Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-a70ee8c8bdc1a6d2d4f1d71303314bd03773e46c sh smoketest.sh

andrewazores · 2022-12-14T21:04:57Z

If I run a previous build of the web-client and it persists some data into localStorage, then run a build with this PR, I get a broken application (full white screen) and devtools console messages like:

Uncaught TypeError: cardConfigs is undefined
    Dashboard 71148:226
    React 21
    91832 npm.react-dom.79f07555ac3ff8c9.bundle.js:48
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    <anonymous> React
    54142 npm.react-dom.79f07555ac3ff8c9.bundle.js:58
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    React 2
    73116 npm.react-dom.79f07555ac3ff8c9.bundle.js:18
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    <anonymous> React
    73935 npm.react-dom.79f07555ac3ff8c9.bundle.js:28
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    <anonymous> 25796:3
    25796 app.55d1a74d04c08471.bundle.js:348
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    __webpack_exec__ app.55d1a74d04c08471.bundle.js:464
    <anonymous> app.55d1a74d04c08471.bundle.js:465
    O runtime.2846e04d228a8620.bundle.js:83
    <anonymous> app.55d1a74d04c08471.bundle.js:466
    webpackJsonpCallback runtime.2846e04d228a8620.bundle.js:1228
    <anonymous> app.55d1a74d04c08471.bundle.js:10

The above error occurred in the <Dashboard> component:

Dashboard@webpack-internal:///71148:219:76
Route@webpack-internal:///16550:647:29
RouteWithTitleUpdates@webpack-internal:///86887:215:21
Switch@webpack-internal:///16550:849:29
Suspense
LastLocationProvider@webpack-internal:///12556:161:47
C@webpack-internal:///16550:904:31
AppRoutes@webpack-internal:///86887:229:54
main
div
DrawerContentBody@webpack-internal:///16537:16:54
div
div
DrawerMain@webpack-internal:///97311:16:34
DrawerContent@webpack-internal:///78135:20:131
div
Drawer@webpack-internal:///5436:30:138
div
div
Page@webpack-internal:///16488:37:9
AppLayout@webpack-internal:///90428:103:20
Router@webpack-internal:///16550:266:30
BrowserRouter@webpack-internal:///73727:57:35
Provider@webpack-internal:///60682:14:18
App

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://reactjs.org/link/error-boundaries to learn more about error boundaries.

Uncaught TypeError: cardConfigs is undefined
    Dashboard 71148:226
    React 21
    91832 npm.react-dom.79f07555ac3ff8c9.bundle.js:48
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    <anonymous> React
    54142 npm.react-dom.79f07555ac3ff8c9.bundle.js:58
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    React 2
    73116 npm.react-dom.79f07555ac3ff8c9.bundle.js:18
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    <anonymous> React
    73935 npm.react-dom.79f07555ac3ff8c9.bundle.js:28
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    fn runtime.2846e04d228a8620.bundle.js:331
    <anonymous> 25796:3
    25796 app.55d1a74d04c08471.bundle.js:348
    __webpack_require__ runtime.2846e04d228a8620.bundle.js:36
    __webpack_exec__ app.55d1a74d04c08471.bundle.js:464
    <anonymous> app.55d1a74d04c08471.bundle.js:465
    O runtime.2846e04d228a8620.bundle.js:83
    <anonymous> app.55d1a74d04c08471.bundle.js:466
    webpackJsonpCallback runtime.2846e04d228a8620.bundle.js:1228
    <anonymous> app.55d1a74d04c08471.bundle.js:10

Going into browser settings and clearing localstorage for localhost fixes it. It makes sense that the data fails to load in this case since the key it's stored under is different and the data may be laid out differently within the storage, but is there anything we can do to catch this and just clear or at least ignore the invalid state so the application doesn't end up broken?

andrewazores · 2022-12-14T21:11:01Z

^ I don't think that behaviour is unique to this PR though, but it might be a good opportunity to address it as well.

tthvo · 2022-12-14T21:11:40Z

Oh yes there is actually a feature in redux-persist that does migrating previous state. I will look into it and do a simpler version of it.

tthvo · 2022-12-14T21:15:01Z

I am thinking of doing versioning:

Add an optional _version field in the reducer state.
Then, if there is a version mismatch, we can just discard the version in the localStorage.
Anytime we have a new version of the store state, we just need to increment the version.

What do you think?

andrewazores · 2022-12-14T21:20:22Z

Sounds like a reasonable plan to me. Would the version be for the store as a whole or per slice? Losing AA card filter states on version updates isn't so bad, but losing dashboard layouts would be very unfortunate.

tthvo · 2022-12-14T21:21:56Z

I think versioning per slice is more convenient as changes mostly just tough a single slice.

andrewazores · 2022-12-14T21:26:23Z

Agreed, sounds good. I suppose there could actually be a top-level version too which follows the same logic. It would be unlikely to change, but if it did, we would be glad it's versioned.

tthvo · 2022-12-14T21:43:26Z

I suppose there could actually be a top-level version too which follows the same logic. It would be unlikely to change, but if it did, we would be glad it's versioned.

Ahh got a little unfortunate here as store state is constructed from reducers. Adding a custom field (i.e. _version) on root state is not allowed... I guess we could just version slices then.

tthvo · 2022-12-14T21:47:50Z

There is no clean up for localStorage if there is a version mismatch. I think we should let it be in case the user switches to an older version of web console that uses that version.

github-actions · 2022-12-14T21:50:52Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-d691d765ed7b849a5e0d0af43e3b7f86963964e5 sh smoketest.sh

andrewazores · 2022-12-15T00:25:37Z

It would probably only be developers who downgrade to an older version, not end users, so I wouldn't worry too much about trying to maintain storage compatibility in both directions.

src/app/Shared/Redux/Filters/RecordingFilterSlice.tsx

github-actions · 2022-12-15T00:47:54Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-8879776de4df4199808f6dfb29c662509be5769b sh smoketest.sh

Signed-off-by: Thuan Vo <thvo@redhat.com>

github-actions · 2022-12-15T02:05:17Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-753-88c587bca46321e1b68d452add01c99c4232a6ea sh smoketest.sh

tthvo added the chore Refactor, rename, cleanup, etc. label Dec 13, 2022

tthvo requested review from andrewazores and ebaron as code owners December 13, 2022 06:45

github-actions bot added dependent needs-triage Needs thorough attention from code reviewers labels Dec 13, 2022

mergify bot added the safe-to-test label Dec 13, 2022

tthvo requested a review from maxcao13 December 13, 2022 06:46

maxcao13 mentioned this pull request Dec 13, 2022

[Task] Apply yarn format:apply to ./src/ #740

Closed

5 tasks

tthvo force-pushed the redux-persist branch from de7a826 to 9d48105 Compare December 13, 2022 20:23

github-actions bot removed the dependent label Dec 13, 2022

tthvo force-pushed the redux-persist branch 2 times, most recently from 3a07aea to 2f05779 Compare December 13, 2022 20:26

tthvo removed the needs-triage Needs thorough attention from code reviewers label Dec 13, 2022

tthvo marked this pull request as draft December 13, 2022 20:42

github-actions bot added the needs-triage Needs thorough attention from code reviewers label Dec 13, 2022

tthvo changed the title ~~perf(redux): use redux-persist to handle localStorage~~ perf(redux): avoid persisting entire store on individual state change Dec 13, 2022

tthvo force-pushed the redux-persist branch from f38c62a to e309445 Compare December 13, 2022 21:18

andrewazores mentioned this pull request Dec 13, 2022

[Request] Persist notifications #755

Open

tthvo marked this pull request as ready for review December 13, 2022 23:19

andrewazores reviewed Dec 14, 2022

View reviewed changes

src/app/Shared/Redux/Middlewares/PersistMiddleware.tsx Show resolved Hide resolved

tthvo changed the title ~~perf(redux): avoid persisting entire store on individual state change~~ perf(redux): optimize state persist and add slice versions Dec 14, 2022

andrewazores reviewed Dec 15, 2022

View reviewed changes

src/app/Shared/Redux/Filters/RecordingFilterSlice.tsx Outdated Show resolved Hide resolved

andrewazores approved these changes Dec 15, 2022

View reviewed changes

tthvo added 6 commits December 14, 2022 20:59

perf(redux): avoid persisting entire store on individual state change

206779e

Signed-off-by: Thuan Vo <thvo@redhat.com>

chore(imports): fix circular deps imports

de9a3f0

chore(redux): rename action intents

05498f7

fixup(redux): add warning message

8ab7423

feat(redux): add versioning to slice

1a2fc26

chore(redux): provide default to getPersistedState

88c587b

andrewazores force-pushed the redux-persist branch from 8879776 to 88c587b Compare December 15, 2022 01:59

andrewazores merged commit b2c1884 into cryostatio:main Dec 15, 2022

tthvo deleted the redux-persist branch April 5, 2023 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(redux): optimize state persist and add slice versions #753

perf(redux): optimize state persist and add slice versions #753

tthvo commented Dec 13, 2022 •

edited

Loading

tthvo commented Dec 13, 2022 •

edited

Loading

github-actions bot commented Dec 13, 2022

andrewazores commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

tthvo commented Dec 13, 2022

andrewazores commented Dec 13, 2022

tthvo commented Dec 13, 2022

andrewazores commented Dec 13, 2022

andrewazores commented Dec 13, 2022

andrewazores commented Dec 13, 2022

tthvo commented Dec 13, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022

tthvo commented Dec 14, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022

github-actions bot commented Dec 14, 2022

andrewazores commented Dec 14, 2022

andrewazores commented Dec 14, 2022

tthvo commented Dec 14, 2022

tthvo commented Dec 14, 2022 •

edited

Loading

andrewazores commented Dec 14, 2022

tthvo commented Dec 14, 2022

andrewazores commented Dec 14, 2022

tthvo commented Dec 14, 2022 •

edited

Loading

tthvo commented Dec 14, 2022

github-actions bot commented Dec 14, 2022

andrewazores commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

perf(redux): optimize state persist and add slice versions #753

perf(redux): optimize state persist and add slice versions #753

Conversation

tthvo commented Dec 13, 2022 • edited Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Description of the change:

Motivation for the change:

tthvo commented Dec 13, 2022 • edited Loading

github-actions bot commented Dec 13, 2022

andrewazores commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

tthvo commented Dec 13, 2022

andrewazores commented Dec 13, 2022

tthvo commented Dec 13, 2022

andrewazores commented Dec 13, 2022

andrewazores commented Dec 13, 2022

andrewazores commented Dec 13, 2022

tthvo commented Dec 13, 2022 • edited Loading

github-actions bot commented Dec 14, 2022

tthvo commented Dec 14, 2022 • edited Loading

github-actions bot commented Dec 14, 2022

github-actions bot commented Dec 14, 2022

andrewazores commented Dec 14, 2022

andrewazores commented Dec 14, 2022

tthvo commented Dec 14, 2022

tthvo commented Dec 14, 2022 • edited Loading

andrewazores commented Dec 14, 2022

tthvo commented Dec 14, 2022

andrewazores commented Dec 14, 2022

tthvo commented Dec 14, 2022 • edited Loading

tthvo commented Dec 14, 2022

github-actions bot commented Dec 14, 2022

andrewazores commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

tthvo commented Dec 13, 2022 •

edited

Loading

tthvo commented Dec 13, 2022 •

edited

Loading

tthvo commented Dec 13, 2022 •

edited

Loading

tthvo commented Dec 14, 2022 •

edited

Loading

tthvo commented Dec 14, 2022 •

edited

Loading

tthvo commented Dec 14, 2022 •

edited

Loading