Repository navigation
[No QA] Consolidate on the OXC React Compiler and delete the Babel one - #102659
elirangoshen wants to merge 10 commits into
Conversation
Every build lane now compiles app source with OXC: web through Rsbuild, native through Re.Pack (HybridApp release builds too, since Mobile-Expensify Expensify#14144) and Jest through oxcJestTransformer. The Babel React Compiler only remained on the BUNDLER=metro debug fallback, plus a layer of tooling that ran both compilers and compared them. - Drop babel-plugin-react-compiler from babel.config.js, package.json and knip.json, and inline reactCompilerConfig.js into oxcReactCompilerConfig.js. - Delete checkBoth, checkWithBabel, check-both-compilers-runner, the divergence audit script and their tests. - Reduce the compliance check to OXC: new files must compile and modified files must not regress. The divergence rule and the inline Babel analysis are gone. - Point the lint worker at checkWithOxc and trim the fingerprint files. - Update CLAUDE.md, REACT_COMPILER.md and the compiler coding rule. BUNDLER=metro builds now run without React Compiler memoization. Metro is a debug escape hatch, not a shipped path.
|
|
CI runs Jest with --coverage, and oxcJestTransformer sends app source to babel-jest for those runs so istanbul can instrument in the same pass. That lane still compiled with babel-plugin-react-compiler through babel.config.js. Removing the plugin left CI tests unmemoized, and several UI suites looped until they timed out or ran out of memory. babel.config.js now applies the plugin only for babel-jest, with the options from oxcReactCompilerConfig so both compilers compile the same way. Metro and Re.Pack stay without it. The dependency and its override are restored.
Coverage runs go OXC -> one light Babel pass (istanbul + CJS conversion) using OXC's source map, replacing the babel-jest coverage lane. Timed ~par in an A/B (Oct-5); the report shape shifts (type-only modules drop out, branch totals move), so the Codecov baseline needs team sign-off before merging.
oxcJestTransformer imports babel-plugin-istanbul and the two Babel transform plugins directly now, but they were only transitive deps. CI's knip comparison flagged all three as unlisted.
…e-babel-react-compiler # Conflicts: # package-lock.json # package.json
…e-babel-react-compiler
|
ok https://github.com/Expensify/Mobile-Expensify/pull/14186 I suggest to wait few days after https://github.com/Expensify/Mobile-Expensify/pull/14186 will reach prod to make it safer to merge this pr that build upon it. |
Codecov Report✅ All modified and coverable lines are covered by tests. |
Coverage runs compile app source with OXC and only use Babel for istanbul and the CommonJS conversion, with babel.config.js turned off. The jest block's babel-plugin-react-compiler only reached mocks, jest setup files and the OXC fallback, none of which need memoization. Remove it along with the dependency, its override and the knip ignore, and drop the coverage exception from the docs. babel-plugin-react-compiler stays in the lockfile as a dependency of babel-preset-expo, which we only use for inline-env-vars.
About the Codecov report above: the numbers shift because coverage is now measured on OXC output instead of Babel's, not because any test changed. Type-only files compile to nothing and drop out of the report, so totals move and Codecov shows ~4261 files with indirect changes. CI stays green, since Codecov checks are informational in this repo. |
…e-babel-react-compiler # Conflicts: # package.json
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.mp4iOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f398d10523
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .update(esbuild.version) | ||
| .update(oxcVersion) | ||
| .update(transformOptions?.instrument ? 'instrument' : '') |
There was a problem hiding this comment.
When package-lock.json changes one of the newly invoked Babel/Istanbul packages, .github/workflows/test.yml restores an older .jest-cache via its restore-keys entry on line 43, but this custom key only distinguishes the OXC/esbuild versions and the instrumentation bit. Unchanged source files can therefore reuse output and coverage counters generated by an older @babel/core, babel-plugin-istanbul, or CommonJS/dynamic-import plugin, mixing stale instrumentation with the current dependencies; include those package versions (or a lockfile fingerprint) in the instrumented key.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| return ['metro', 'babel-jest'].includes(runningIn) ? metro : {}; | ||
| return runningIn === 'babel-jest' || runningIn === 'metro' ? metro : {}; |
There was a problem hiding this comment.
BUNDLER=metro now builds without React Compiler
The metro lane in babel.config.js lost babel-plugin-react-compiler and nothing replaces it, since OXC only runs in the Re.Pack loader. BUNDLER=metro is still advertised as the fallback (here and in Mobile-Expensify's build.gradle and react-native-xcode.sh), so anyone using it would ship an unmemoized bundle with no error or warning.
Lines 8 to 11 in f398d10
- On this PR run
BUNDLER=metro npx rock bundle --platform ios --dev false --entry-file index.js --bundle-output /tmp/main.jsbundle - Run
grep -o memo_cache_sentinel /tmp/main.jsbundle | wc -l
Expected: thousands, or the build refuses to run
Actual: 0
| .update(REACT_COMPILER_CONFIG_KEY) | ||
| .update(esbuild.version) | ||
| .update(oxcVersion) | ||
| .update(transformOptions?.instrument ? 'instrument' : '') |
There was a problem hiding this comment.
Coverage cache key ignores Babel and istanbul versions
The instrument lane output depends on @babel/core, babel-plugin-istanbul and @babel/plugin-transform-modules-commonjs, but none of their versions are hashed. After a bump, a local .jest-cache keeps serving stale instrumented output.
| function processWithOxcForCoverage(sourceText, sourcePath, transformOptions) { | ||
| const {transformSync} = getOxcPipeline(); | ||
| const oxcResult = transformSync(sourcePath, sourceText, { | ||
| lang: getLang(sourcePath), | ||
| sourcemap: true, | ||
| jsx: {runtime: 'automatic', development: true}, | ||
| reactCompiler: REACT_COMPILER_OPTIONS, | ||
| }); | ||
|
|
||
| if (oxcResult.fatal || !oxcResult.code) { | ||
| return null; | ||
| } | ||
| return !NODE_MODULES_RE.test(filename) && !TESTS_RE.test(filename) && !JEST_SETUP_RE.test(filename) && !MOCKS_RE.test(filename) && !TEST_FILE_RE.test(filename); | ||
|
|
||
| const result = require('@babel/core').transformSync(oxcResult.code, { | ||
| filename: sourcePath, | ||
| configFile: false, | ||
| babelrc: false, | ||
| inputSourceMap: oxcResult.map, | ||
| sourceMaps: true, | ||
| auxiliaryCommentBefore: ' istanbul ignore next ', | ||
| plugins: [ | ||
| [require.resolve('babel-plugin-istanbul'), {cwd: transformOptions.config.cwd, exclude: []}], | ||
| [require.resolve('@babel/plugin-transform-modules-commonjs'), {strictMode: false}], | ||
| require.resolve('@babel/plugin-transform-dynamic-import'), | ||
| ], | ||
| }); | ||
|
|
||
| return result?.code ? {code: result.code, map: result.map} : null; | ||
| } | ||
|
|
||
| function processWithOxc(sourceText, sourcePath) { |
There was a problem hiding this comment.
Duplicated OXC call
processWithOxcForCoverage repeats the transformSync call and fatal check from processWithOxc. A shared helper keeps the two lanes from drifting.
|
|
|
@MelvinBot review |
ReviewThe PR looks correct apart from one issue to fix before merge. CI is green except for the checklist checks. I agree with all three of your inline comments and found nothing new beyond one stale comment. 1. Fix before merge: I confirmed it. The
Either way, merge only after HybridApp stops bundling through Metro, as 2. Should fix: coverage cache key misses Babel and istanbul versions (your inline comment)
3. Nit: duplicated OXC call (your inline comment)
What I checked and found fine
|
fixed |
|
I addressed all comments @aimane-chnaif |
1. Import comments contradict each otherThe worker says Bun resolves the App/scripts/lint/processors/ReactCompilerWorker.ts Lines 1 to 3 in e0c996a App/scripts/react-compiler-compliance-check.ts Lines 16 to 21 in e0c996a Fix: one wording in both files, placed above the first import. 2.
|
Re-review at
|
| # | Comment | Verdict |
|---|---|---|
| 1 | Import comments contradict each other | ✅ Valid. ReactCompilerWorker.ts:1 says "without the extension" above an import that uses .mjs. |
| 2 | Stale metro naming and comments |
✅ Valid. babel.config.js:4 and :171 still describe Metro as a live caller. You can defer the rename. |
| 3 | istanbul cwd should be rootDir |
❌ Not needed. The installed babel-jest 29.7.0 passes cwd: transformOptions.config.cwd to babel-plugin-istanbul, not rootDir, so oxcJestTransformer.js:144 already matches it. A short comment on strictMode: false would still help. |
| 4 | Coverage lane hashes esbuild.version |
✅ Valid nit. oxcJestTransformer.js:190 hashes it on both lanes, so an esbuild bump causes an extra cache miss on coverage runs. getOxcPipeline still loads esbuild in both lanes. |
| 5 | Redundant CompilationResult | undefined annotation |
✅ Valid nit. TypeScript infers the same type. |
|
Some minor nits above but not blockers |
Code quality review at
|
Explanation of Change
Every build now compiles app source with the OXC React Compiler: web through Rsbuild, native through Re.Pack (HybridApp release builds too, since https://github.com/Expensify/Mobile-Expensify/pull/14186), and Jest through
oxcJestTransformer(#102081). The Babel React Compiler was left on theBUNDLER=metrofallback, on Jest coverage runs, and in a set of tools that ran both compilers and compared them. This PR moves coverage runs onto OXC, removesbabel-plugin-react-compilercompletely and deletes the tools.Jest coverage runs use OXC too. CI runs Jest with
--coverage, and those runs used to go throughbabel-jest, which is why the Babel React Compiler had to stay. Now OXC compiles the file first, and a small Babel pass only adds coverage counters. It runs about as fast as before. Sobabel-plugin-react-compileris removed frombabel.config.js,package.jsonandknip.json(it stays in the lockfile only becausebabel-preset-expodepends on it).One set of compiler options.
config/babel/reactCompilerConfig.jsis folded intooxcReactCompilerConfig.js, which every lane (Rsbuild, Re.Pack, Jest and the checker) now uses.Tools that compared the two compilers deleted.
checkBoth,checkWithBabel,check-both-compilers-runner, the divergence audit script and their tests.Compliance check is OXC only. It keeps the two rules that matter: new files must compile, and modified files must not regress. The divergence rule and the inline Babel analysis are gone. The check's test runs under
bun:testnow, so it imports the OXC checker directly instead of through a dynamic import.Lint uses one compiler.
ReactCompilerWorkercallscheckWithOxcdirectly, so lint no longer runs a second compiler for each file. The cache fingerprint only lists the files that still exist.CLAUDE.md,
contributingGuides/REACT_COMPILER.mdand the compiler coding rule now describe OXC as the only compiler.Fixed Issues
$ #101668
PROPOSAL:
Automated Tests
tests/tooling/ReactCompilerComplianceCheck.test.tsnow runs against OXC. The error-location test checks the line that breaks the rule, since OXC reports that line and not the enclosing function.tests/unit/CheckBothCompilersTest.tsis deleted with the code it covered.--coverage, so they now test the OXC coverage path. Locally,npx jest tests/ui --coveragepasses all 392 suites withTZ=UTC.Tests
npm run react-compiler-compliance-check -- --verbose check src/pages/home/and verify every file isCOMPILEDorSKIPPED, noneFAILED, and nothing mentions Babel.npm run lintandnpm run test:bun, and verify both are green.npx rock bundle --platform ios --dev false --entry-file index.js --bundle-output /tmp/main.jsbundleand verify the bundle contains React Compiler output:grep -o memo_cache_sentinel /tmp/main.jsbundle | wc -lis in the thousands.npm run ios, open Home, then FAB → Create expense, and switch between the Manual and Scan tabs. Verify nothing crashes.Offline tests
N/A. Build and lint tooling only; app source compiles the same way it did before.
QA Steps
N/A. Build and lint tooling only.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari