build: Enable React Compiler for Browserify builds, fix react-compiler/react-compiler ESLint rule violations
#37480
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
Collaborator
✨ 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, +623 -301)
💎 @MetaMask/metamask-assets (2 files, +2 -4)
📜 @MetaMask/policy-reviewers (8 files, +623 -301)
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, +623 -301)
|
Collaborator
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
Contributor
Author
|
@david0xd I've addressed or closed all cursorbot comments. Almost all of them are pointing out issues with the preexisting code not new additions from this PR. Some of them are plain wrong. Hopefully this makes things less noisy.😅 |
| }, | ||
| }, | ||
| plugins: ['react'], | ||
| plugins: ['react', 'react-compiler'], |
Contributor
There was a problem hiding this comment.
Is react-compiler the right plugin? https://react.dev/blog/2025/10/07/react-compiler-1 says
Compiler-powered lint rules ship in eslint-plugin-react-hooks’s recommended and recommended-latest preset.
and
If you have already installed eslint-plugin-react-compiler, you can now remove it and use eslint-plugin-react-hooks@latest. Many thanks to @michaelfaith for contributing to this improvement!
Contributor
Author
There was a problem hiding this comment.
I was planning to do this in a separate PR since there would be some noisy diffs e.g. removing eslint-disable-next-line react-compiler/react-compiler. More importantly, our eslint-plugin-react-hooks is two major versions behind. I haven't looked into what the breaking changes are between ours and the latest version, but I'm cautious about attempting that upgrade here.
For this PR we're using eslint-plugin-react-compiler@19.1.0-rc.2. Based on its changelog, it appears that eslint-plugin-react-hooks just adds the existing react compiler lint rules without altering them, so we shouldn't be missing anything functionality-wise for now.
Collaborator
Builds ready [e839433]
UI Startup Metrics (1251 ± 99 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| "react": "^17.0.2", | ||
| "react-beautiful-dnd": "^13.1.1", | ||
| "react-chartjs-2": "^5.2.0", | ||
| "react-compiler-runtime": "^1.0.0", |
There was a problem hiding this comment.
Bug: Incorrect semver range for react-compiler-runtime dependency (Bugbot Rules)
The react-compiler-runtime dependency uses ^1.0.0 which allows minor and patch version updates. However, as noted in the PR discussion, this package is in the 0.x version range where breaking changes can occur at any time per semver conventions. The version should be pinned to exactly 0.2.0 (without the caret) to prevent unexpected breaking changes from being automatically installed.
Contributor
Author
There was a problem hiding this comment.
Nope that discussion was about a different package (react-compiler-webpack) that's no longer added in this PR. ^1.0.0 is not in the 0.x range so the note about using an exact version doesn't apply here. Please stop bringing this up.
Contributor
Author
|
@metamaskbot update-policies |
e628f03 to
e839433
Compare
Collaborator
|
No policy changes |
Collaborator
Builds ready [e839433]
UI Startup Metrics (1215 ± 101 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Collaborator
Builds ready [2576d4f]
UI Startup Metrics (1206 ± 101 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
7 tasks
Contributor
Author
|
@metamaskbot update-policies |
Collaborator
|
Policy update failed. You can review the logs or retry the policy update here |
Contributor
Author
|
The repository-health-checks job is failing on the non-blocking dedupe check because... Running |
Collaborator
Builds ready [d0321db]
UI Startup Metrics (1227 ± 118 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| ) { | ||
| if (action.ownerId === undefined) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Bug: Inconsistent undefined handling breaks SET_ALERT_CONFIRMED action
The reducer returns early when action.ownerId is undefined for all action types, but SetAlertConfirmedAction has ownerId typed as string (not string | undefined). This creates an inconsistency where the early return check applies to an action type that should never have an undefined ownerId. If setAlertConfirmed is somehow called with an undefined value that gets coerced, the SET_ALERT_CONFIRMED case will be unreachable, silently failing to update the confirmed state without any error.
95de3a7 to
713aa39
Compare
| ) { | ||
| if (action.ownerId === undefined) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Bug: Reducer silently ignores valid SET_ALERT_CONFIRMED actions
The reducer now returns early when action.ownerId is undefined for all action types, but SetAlertConfirmedAction has ownerId typed as string (not string | undefined), meaning it can never be undefined. However, the early return applies to all actions including SET_ALERT_CONFIRMED. If a SET_ALERT_CONFIRMED action somehow has an undefined ownerId at runtime (despite the type), it will silently return the unchanged state instead of processing the action. This inconsistency between the type definition and runtime check creates a potential silent failure mode where alert confirmations could be lost without any error indication.
Collaborator
Builds ready [713aa39]
UI Startup Metrics (1208 ± 110 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This commit enables React Compiler for 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 in new code.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 PRs.
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/chromeopen a fileui-3.js-ui-17.js._reactCompilerRuntime.c".The 10% bundle size increase is also an indication of the extra memoization being applied.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Enable React Compiler and its ESLint rule, updating build/config and policies, and refactor code/tests (hooks/memoization, APIs) to satisfy the compiler.
babel-plugin-react-compiler) forui/{components,contexts,hooks,layouts,pages}; addreact-compiler-runtime.eslint-plugin-react-compilerrule; bumpeslint-plugin-reactandeslint-plugin-react-hooks.react-compiler-runtime,babel-plugin-react-compiler, and updated ESLint deps across browserify/webpack/build-system policies.Array.isArray, optional chaining, immutable updates, prop/types tweaks, background method check).useSafeChainsListValidationSelector→getUseSafeChainsListValidation,use4ByteResolutionSelector→getUse4ByteResolution,getIsEstimatedReturnLow→useGetIsEstimatedReturnLow, etc.isNativeAsset), balance handling made immutable.renderHookand adapted mocks for renamed selectors/components.Written by Cursor Bugbot for commit 713aa39. This will update automatically on new commits. Configure here.