chore: replace proxy-memoize with reselect #15333
Conversation
|
🚀 Expo preview is ready!
|
169420a to
9670ecd
Compare
| @@ -147,303 +150,300 @@ export const prepareAccountsReducer = createReducerWithExtraDeps( | |||
| }, | |||
| ); | |||
|
|
|||
| export const selectAccounts = (state: AccountsRootState) => state.wallet.accounts; | |||
| const createAppSelector = createWeakMapSelector.withTypes< | |||
There was a problem hiding this comment.
why is this has ...App... in the name?
There was a problem hiding this comment.
What about changing it to createMemoizedSelector?
There was a problem hiding this comment.
It's taken from their docs, but createMemoizedSelector looks good
| @@ -66,7 +65,7 @@ export type TransactionsRootState = { | |||
| transactions: { [key: AccountKey]: (WalletAccountTransaction | null | undefined)[] }; | |||
| }; | |||
| }; | |||
| }; | |||
| } & AccountsRootState; | |||
There was a problem hiding this comment.
Does this mean that it will be sufficient to send a only one RootState type to the useSelector() call? Love it 👏
There was a problem hiding this comment.
Yea, I don't see reason why not to do this
suite-native/app/app.config.ts
Outdated
| @@ -1,4 +1,4 @@ | |||
| /* eslint-disable no-console */ | |||
There was a problem hiding this comment.
This should be probably returned
There was a problem hiding this comment.
It doesn't throw anywhere so I guess it's fine
| export const selectAccountTokenTransactions = createAppSelector( | ||
| [ | ||
| selectAccountTransactions, | ||
| (_state, _accountKey: AccountKey, tokenAddress: TokenAddress) => tokenAddress, |
There was a problem hiding this comment.
Damn, this is so ugly that you have to pass a function instead of a simple argument value 🥴. But it is how it is
There was a problem hiding this comment.
It's even worse if you have multiple nested selectors with different params :D
| (_state, domain: ContextDomain) => domain, | ||
| (_state, _domain, language: string) => language, |
There was a problem hiding this comment.
nit:
| (_state, domain: ContextDomain) => domain, | |
| (_state, _domain, language: string) => language, | |
| (_state, domain: ContextDomain, language: string) => {domain, language}, |
There was a problem hiding this comment.
I am not sure, but I think this will break memoization, but IDK
7453135 to
3c00e24
Compare
|
Could you please provide some at least basic PR description why this change is happening? |
3c00e24 to
14579f9
Compare
|
@matejkriz done |
|
QA OK tested on all and academic seed
Info:
|
14579f9 to
8d94d01
Compare
Description
I migrated from
proxy-memoizebecause it was too magical and caused issues that some selectors did not updated correctly randomly and we were also stucked on old version because on new it was even worse and it throwed some errors on mobile too.Reselect is more standart library and it works, even that DX is somewhat worse especially when you have multiple nested parametrized selectors.
Related Issue
Resolve #14785
Screenshots: