-
Notifications
You must be signed in to change notification settings - Fork 5.4k
build: Enable React Compiler for Browserify builds, fix react-compiler/react-compiler ESLint rule violations
#37480
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
build: Enable React Compiler for Browserify builds, fix react-compiler/react-compiler ESLint rule violations
#37480
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. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning MetaMask internal reviewing guidelines:
Ignoring alerts on: |
✨ Files requiring CODEOWNER review ✨🔑 @MetaMask/accounts-engineers (2 files, +8 -8)
👨🔧 @MetaMask/core-extension-ux (1 files, +0 -1)
🫰 @MetaMask/core-platform (1 files, +3 -0)
🎨 @MetaMask/design-system-engineers (2 files, +4 -1)
🧩 @MetaMask/extension-devs (8 files, +812 -451)
💎 @MetaMask/metamask-assets (2 files, +2 -4)
📜 @MetaMask/policy-reviewers (8 files, +812 -451)
Tip Follow the policy review process outlined in the LavaMoat Policy Review Process doc before expecting an approval from Policy Reviewers. 🔗 @MetaMask/supply-chain (8 files, +812 -451)
|
Builds ready [3eabf52]
UI Startup Metrics (1311 ± 102 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/helpers/higher-order-components/with-router-hooks/with-router-hooks.tsx
Outdated
Show resolved
Hide resolved
189e390
| tokenToFiatConversionRate: Numeric | undefined, | ||
| ) { | ||
| const tokenToFiatConversionRateToString = | ||
| tokenToFiatConversionRate?.toString(); |
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.
Bug: Dependency array excludes object causing stale closures
The useCallback dependency array uses tokenToFiatConversionRateToString instead of tokenToFiatConversionRate, but the callback function body references tokenToFiatConversionRate directly. This creates a stale closure where the callback captures an old version of tokenToFiatConversionRate even when its value changes, because toString() might return the same string for different Numeric object instances with the same value. The callback will use outdated conversion rates, causing incorrect currency calculations.
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.
toString() might return the same string for different Numeric object instances with the same value
- Yes, we're intentionally taking advantage of this.
- We're prioritizing preserving existing behavior. We want to minimize disruptive changes.
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.
WONTFIX: Preexisting issue
Bug: Missing dependency in useMemo causes stale filtering
The sortedFilteredTokens useMemo hook uses shouldHideZeroBalanceTokens in its filtering logic on line 117 to determine whether to hide tokens with zero balance, but this variable is missing from the dependency array. When shouldHideZeroBalanceTokens changes, the memoized value won't recompute, causing the token list to continue showing or hiding zero-balance tokens based on the stale value until another dependency triggers a recomputation.
ui/components/app/assets/token-list/token-list.tsx#L92-L151
metamask-extension/ui/components/app/assets/token-list/token-list.tsx
Lines 92 to 151 in be6e749
| const sortedFilteredTokens = useMemo(() => { | |
| if (!isMultichainAccountsState2Enabled) { | |
| const balances = isEvm ? evmBalances : multichainAssets; | |
| const filteredAssets = filterAssets(balances as TokenWithFiatAmount[], [ | |
| { | |
| key: 'chainId', | |
| opts: isEvm ? networksToShow : { [currentNetwork.chainId]: true }, | |
| filterCallback: 'inclusive', | |
| }, | |
| ]); | |
| return sortAssets([...filteredAssets], tokenSortConfig); | |
| } | |
| const accountAssetsPreSort = Object.entries(accountGroupIdAssets).flatMap( | |
| ([chainId, assets]) => { | |
| if (!allEnabledNetworksForAllNamespaces.includes(chainId)) { | |
| return []; | |
| } | |
| // Mapping necessary to comply with the type. Fields will be overriden with useTokenDisplayInfo | |
| return assets.filter((asset) => { | |
| if (isTronResource(asset)) { | |
| return false; | |
| } | |
| if (shouldHideZeroBalanceTokens && asset.balance === '0') { | |
| return false; | |
| } | |
| return true; | |
| }); | |
| }, | |
| ); | |
| const accountAssets = sortAssetsWithPriority( | |
| accountAssetsPreSort, | |
| tokenSortConfig, | |
| ); | |
| return accountAssets.map((asset) => { | |
| const token: TokenWithFiatAmount = { | |
| ...asset, | |
| tokenFiatAmount: asset.fiat?.balance, | |
| secondary: null, | |
| title: asset.name, | |
| address: 'address' in asset ? asset.address : (asset.assetId as Hex), | |
| chainId: asset.chainId as Hex, | |
| }; | |
| return token; | |
| }); | |
| }, [ | |
| isEvm, | |
| evmBalances, | |
| multichainAssets, | |
| networksToShow, | |
| currentNetwork.chainId, | |
| tokenSortConfig, | |
| isMultichainAccountsState2Enabled, | |
| accountGroupIdAssets, | |
| // newTokensImported included in deps, but not in hook's logic |
Builds ready [be6e749]
UI Startup Metrics (1280 ± 123 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
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.
WONTFIX: Preexisting issue
Bug: Missing dependencies in useEffect dependency array
The useEffect hook uses isInputUnchanged and isMatchingUpstream variables inside the effect (line 146) but they are missing from the dependency array. The effect calculates isUpstreamValue using these variables, which affects the behavior of processNewDecimalValue. When these values change, the effect won't re-run, causing stale values to be used and potentially incorrect currency conversion calculations. The removal of the eslint-disable comment for react-hooks/exhaustive-deps exposed this existing issue that should be fixed.
ui/components/app/currency-input/currency-input.js#L166-L174
metamask-extension/ui/components/app/currency-input/currency-input.js
Lines 166 to 174 in dda7ecb
| // tokenDecimalValue does not need to be in here, since this side effect is only for upstream updates | |
| }, [ | |
| hexValue, | |
| asset?.address, | |
| processNewDecimalValue, | |
| isTokenPrimary, | |
| assetDecimals, | |
| isDisabled, | |
| ]); |
|
Hey @davidmurdoch following up on the requested changes!
|
Builds ready [dda7ecb]
UI Startup Metrics (1306 ± 132 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
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.
WONTFIX: Preexisting issue
Bug: Missing dependency causes stale token list rendering
The shouldHideZeroBalanceTokens variable is used inside the useMemo hook at line 119 to filter assets, but it's missing from the dependency array at lines 144-157. When users toggle the "hide zero balance tokens" setting, the token list won't update to reflect the new preference until another dependency changes, causing the UI to display stale data that doesn't match the user's current settings.
ui/components/app/assets/token-list/token-list.tsx#L92-L157
metamask-extension/ui/components/app/assets/token-list/token-list.tsx
Lines 92 to 157 in 2455a12
| ); | |
| const sortedFilteredTokens = useMemo(() => { | |
| if (!isMultichainAccountsState2Enabled) { | |
| const balances = isEvm ? evmBalances : multichainAssets; | |
| const filteredAssets = filterAssets(balances as TokenWithFiatAmount[], [ | |
| { | |
| key: 'chainId', | |
| opts: isEvm ? networksToShow : { [currentNetwork.chainId]: true }, | |
| filterCallback: 'inclusive', | |
| }, | |
| ]); | |
| return sortAssets([...filteredAssets], tokenSortConfig); | |
| } | |
| const accountAssetsPreSort = Object.entries(accountGroupIdAssets).flatMap( | |
| ([chainId, assets]) => { | |
| if (!allEnabledNetworksForAllNamespaces.includes(chainId)) { | |
| return []; | |
| } | |
| // Mapping necessary to comply with the type. Fields will be overriden with useTokenDisplayInfo | |
| return assets.filter((asset) => { | |
| if (isTronResource(asset)) { | |
| return false; | |
| } | |
| if (shouldHideZeroBalanceTokens && asset.balance === '0') { | |
| return false; | |
| } | |
| return true; | |
| }); | |
| }, | |
| ); | |
| const accountAssets = sortAssetsWithPriority( | |
| accountAssetsPreSort, | |
| tokenSortConfig, | |
| ); | |
| return accountAssets.map((asset) => { | |
| const token: TokenWithFiatAmount = { | |
| ...asset, | |
| tokenFiatAmount: asset.fiat?.balance, | |
| secondary: null, | |
| title: asset.name, | |
| address: 'address' in asset ? asset.address : (asset.assetId as Hex), | |
| chainId: asset.chainId as Hex, | |
| }; | |
| return token; | |
| }); | |
| }, [ | |
| isEvm, | |
| evmBalances, | |
| multichainAssets, | |
| networksToShow, | |
| currentNetwork.chainId, | |
| tokenSortConfig, | |
| isMultichainAccountsState2Enabled, | |
| accountGroupIdAssets, | |
| // newTokensImported included in deps, but not in hook's logic | |
| newTokensImported, | |
| allEnabledNetworksForAllNamespaces, | |
| ]); | |
Builds ready [2455a12]
UI Startup Metrics (1309 ± 130 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [9e269b9]
UI Startup Metrics (1332 ± 142 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
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.
WONTFIX: Preexisting issue
Bug: Missing dependency in useMemo causes stale closure
The useMemo hook uses shouldHideZeroBalanceTokens inside its callback at line 119 to filter assets, but this variable is not included in the dependency array at lines 145-157. This creates a stale closure where changes to shouldHideZeroBalanceTokens won't trigger recalculation of sortedFilteredTokens, causing the UI to display incorrect token lists when users toggle the zero balance filter setting.
ui/components/app/assets/token-list/token-list.tsx#L144-L157
metamask-extension/ui/components/app/assets/token-list/token-list.tsx
Lines 144 to 157 in c47e3f6
| }, [ | |
| isEvm, | |
| evmBalances, | |
| multichainAssets, | |
| networksToShow, | |
| currentNetwork.chainId, | |
| tokenSortConfig, | |
| isMultichainAccountsState2Enabled, | |
| accountGroupIdAssets, | |
| // newTokensImported included in deps, but not in hook's logic | |
| newTokensImported, | |
| allEnabledNetworksForAllNamespaces, | |
| ]); | |
Builds ready [c47e3f6]
UI Startup Metrics (1328 ± 134 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Motivation
This commit enables React Compiler for Babel-transformed Browserify builds, and for ESLint (via the
react-compiler/react-compilerruleset). We can expect long-term UI performance benefits and developer time savings from this change. The Compiler transparently adds memoization code for React components and hooks, among other optimizations, and enforces React best practices.Description
Because enabling React Compiler requires addressing all violations of the
react-compiler/react-compilerrule, this commit contains a large number of formatting changes and renames that do not require close review, alongside refactors and logical changes that do.I would suggest that reviewers primarily focus on whether a given change is problematic, rather than on whether it is necessary. If the motivation for any change is unclear, it's safe to assume that it fixes an error thrown in CI. The objective of this commit is to ship fixes that unblock React Compiler without introducing regressions in functionality or performance. Suggestions on implementing React best practices will be catalogued and addressed in follow-up tickets.
The 10% bundle size increase is due to new memoizations being applied as React Compiler transforms files during the build process, which is the tradeoff for improvements in runtime interaction and responsiveness metrics. Despite this change, the UI startup and pageLoad benchmark results show no evidence of degradations in initial load time.
Changelog
CHANGELOG entry: null (non-user facing but regressions possible)
Related issues
react-hooks/rules-of-hooksESLint rule violations #37383Manual testing steps
yarn start../dist/(chrome|firefox)/ui-[3-17].js._reactCompilerRuntime.c".$variable e.g.Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Enables React Compiler in Browserify Babel builds and enforces the react-compiler ESLint rule, with broad hook/memoization refactors, selector renames, tests updates, and dependency/policy changes.
ui/**/{components,contexts,hooks,layouts,pages}withbabel-plugin-react-compiler(target 17).react-compiler-runtimeandbabel-plugin-react-compilerdeps; updateesbuild,koa(v3), and related tooling.react-compiler/react-compilerESLint rule and plugin across JS/TS React configs.useMemostate updates, unstable callbacks, missing keys) withuseCallback/useRef/guards; fix array checks, keys, debounce handling, and effect deps.usePolling,useMultiPolling) with input change detection and cleanup; stabilize navigation and debounce handlers.isNativeAssetand use inAssetPage; compute asset balances immutably.request?.metadata?.idacross confirmations/snaps flows; miscellaneous null-safe checks.useSafeChainsListValidationSelector→getUseSafeChainsListValidation;use4ByteResolutionSelector→getUse4ByteResolution(and propagate).ownerIdoptional handling, asset type helpers).renderHookfor hooks; adjust mocks to renamed selectors and new patterns.Written by Cursor Bugbot for commit c47e3f6. This will update automatically on new commits. Configure here.