-
Notifications
You must be signed in to change notification settings - Fork 5.4k
chore: use stable empty references #36910
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
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [970ded4]
UI Startup Metrics (1250 ± 72 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
When a variable is destructured from ?? {}
, the purpose is to safely assign it with undefined without triggering the "property of undefined" JS TypeError.
Since the reference of the empty object in these cases are never used beyond each destructuring assignment, and also not by React, these aren't cases where the EMPTY_OBJECT
substitution would be useful.
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.
Intermediate variables not used outside of the selector, or downstream by React.
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.
createSelector
memoizes its output based on its input arguments.
So the references of create{,DeepEqual}Selector
output are already stable without needing these changes.
👍 Similar optimizations might be more impactful if they're made in React components and hooks to reactive variables that require referential equality rather than upstream in the selectors. |
970ded4
to
9272811
Compare
Thanks for the review @MajorLift! Updated the objects, but I think the arrays are still valid: |
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [9272811]
UI Startup Metrics (1260 ± 57 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
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.
LGTM! Going forward, React Compiler should take care of this codebase-wide for most relevant cases, but it would be good to see if there are exceptions where manually applying these changes would make a difference for performance.
Description
When selectors return empty arrays or objects using the
??
or||
operators, they create new instances every time the function is called if the value is undefined/null. This breaks reference equality and memoization and can cause excess re-renders.Changelog
CHANGELOG entry: chore: use stable empty references
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Replaces ad-hoc []/{} fallbacks with shared frozen constants across selectors to stabilize memoization and references.
selectAlerts
now returnsEMPTY_ARRAY
fallback.EMPTY_OBJECT
forgetMetamaskState
and in balance-related selectors (account tree, rates, tokens, currency rates) instead of{}
; remove localEMPTY_OBJ
in favor of shared constant.allNftContracts
/allNfts
toEMPTY_OBJECT
.selectNftsByChainId
andselectNonZeroUnusedApprovalsAllowList
now returnEMPTY_ARRAY
when empty.ui/selectors/shared.ts
exporting frozenEMPTY_ARRAY
andEMPTY_OBJECT
; import and use where needed.Written by Cursor Bugbot for commit 9272811. This will update automatically on new commits. Configure here.