perf: blazingly fast #7799
perf: blazingly fast #7799
Conversation
| @@ -54,8 +43,6 @@ const apiMiddleware = [ | |||
|
|
|||
| const subscriptionMiddleware = createSubscriptionMiddleware() | |||
|
|
|||
| const persistedReducer = persistReducer(persistConfig, reducer) | |||
There was a problem hiding this comment.
doesn't persist root anymore
gomesalexandre
changed the title
| 84: clearAssets, | ||
| 85: clearAssets, | ||
| 86: clearAssets, | ||
| 87: clearWalletConnectWalletsMetadata, |
There was a problem hiding this comment.
Converted to the single slice migration format, but since it was a one off, not to the list of "uncomment me"
There was a problem hiding this comment.
Yes, makes a lot of sense
| ...state, | ||
| opportunities: initialState, | ||
| } | ||
| return initialState as OpportunitiesState & PersistPartial |
There was a problem hiding this comment.
[self note]: Test individual migrations to ensure these changes to what's intended (i.e not nuking all cache)
| wallet: { | ||
| ...state.wallet, | ||
| ids: updatedWalletIds, | ||
| byId: updatedWalletById, |
There was a problem hiding this comment.
[comment]: wow now this is 1 level shallower i can see this being faster
| 84: clearAssets, | ||
| 85: clearAssets, | ||
| 86: clearAssets, | ||
| 87: clearWalletConnectWalletsMetadata, |
| ignoredActions: [PERSIST, PURGE], | ||
| }, | ||
| // funnily enough, the checks that should check for perf. issues are actually slowing down the app | ||
| // https://github.com/reduxjs/redux-toolkit/issues/415 |
There was a problem hiding this comment.
[suggestion]: Comment why disabling this doesnt reduce safety so we dont have a panik moment later
| export const clearSnapshotMigrations = { | ||
| // Uncomment me when introducing the first migration for this slice | ||
| // 0: clearSnapshot, | ||
| } as unknown as Omit<MigrationManifest, '_persist'> |
There was a problem hiding this comment.
[blocking question]: What is the default version and should we have it here so the first migration doesnt fail?
There was a problem hiding this comment.
We should not - that will be automagically injected by redux-persist (tested with 0th migrations)
There was a problem hiding this comment.
i.e no migration:
0th migration (don't mind the non-empty slice, devtools acting up and I lost the REHYDRATE event):
| const localWalletSlicePersistConfig = { | ||
| key: 'localWalletSlice', | ||
| storage: localforage, | ||
| } |
There was a problem hiding this comment.
[question]: Should we comment why there is no migration for this slice?
| @@ -89,5 +167,5 @@ export const apiReducers = { | |||
| [abiApi.reducerPath]: abiApi.reducer, | |||
| } | |||
|
|
|||
| export const reducer = combineReducers({ ...sliceReducers, ...apiReducers }) | |||
| export const reducer = combineReducers(Object.assign({}, sliceReducers, apiReducers)) | |||
gomesalexandre
force-pushed
the
feat_split_reducers_persist
branch
from
41895f2
to
de470e2
Compare
Description
Attempt 1 at redux perf. improvements
Issue (if applicable)
Risk
High - touches state. Getting this wrong could brick state, and we'd have to revert this and do a nuke of the whole state.
Testing
Engineering
NOTE: Ensure you check for slice state after rehydration (i.e
{rehydrated: true}for the slice/s you're migrating) to ensure redux-persist rehydrated with your migration:Operations
Screenshots (if applicable)
https://jam.dev/c/6fee8b7a-97d8-48b0-b420-b04f3ef008b4