-
Notifications
You must be signed in to change notification settings - Fork 638
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
Optimize backendNetworks store #6409
Optimize backendNetworks store #6409
Conversation
Not sure why there were two instances of <BackendNetworks />
@@ -154,7 +153,7 @@ const getInitialSliderXPosition = ({ | |||
export const SwapProvider = ({ children }: SwapProviderProps) => { | |||
const { nativeCurrency } = useAccountSettings(); | |||
|
|||
const backendNetworks = useBackendNetworksStore(state => state.backendNetworksSharedValue); | |||
const [nativeChainAssets] = useState(useBackendNetworksStore.getState().getChainsNativeAsset()); |
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.
any reason to not use useRef
here?
@@ -97,6 +98,13 @@ export const TokenToBuyList = () => { | |||
const { internalSelectedInputAsset, internalSelectedOutputAsset, isFetching, isQuoteStale, outputProgress, setAsset } = useSwapContext(); | |||
const { results: sections, isLoading } = useSearchCurrencyLists(); | |||
|
|||
const [supportedChainsBooleanMap] = useState( |
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.
any reason not to use useRef
here?
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.
I think it's mostly a semantic choice, seems a bit cleaner to use useState
since you can omit the setter, making it closer to naturally read only, and with useState
you don't need to deal with .current
.
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.
Sounds reasonable to me. Was just curious!
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.
Insane work. Thanks for cleaning this up. Approved but left a couple small qs
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.
The memoizing is cool. Seems like this file will serve as a good example of how to solve this particular issue going forward. 🫡
What changed (plus any additional context for devs)
backendNetworks
store methods are called thousands of times — this cuts total function execution time by ~95%+~97.5ms
🔴~3.3ms
🟢createQueryStore
and removes the<BackendNetworks />
sync component, which existed in two placesScreen recordings / screenshots
What to test