-
Notifications
You must be signed in to change notification settings - Fork 373
Upgrade to RSPack and pnpm
#9114
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
base: master
Are you sure you want to change the base?
Conversation
835ecec to
4945b72
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9114 +/- ##
==========================================
+ Coverage 80.02% 81.37% +1.35%
==========================================
Files 601 601
Lines 32701 33750 +1049
Branches 3367 3054 -313
==========================================
+ Hits 26168 27465 +1297
+ Misses 6362 5927 -435
- Partials 171 358 +187 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a309230 to
085f554
Compare
|
I was doing a bit more digging. And there are a couple more easy optimizations we can make to improve the test execution and docker. I'll create another commit for that shortly. |
| command: pnpm markdownlint | ||
| name: Check markdown linting | ||
| - run: | ||
| command: yarn test:coverage |
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.
This should remain test:coverage
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.
ok, fixed
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 tried this change, but I think it's causing issues in CI. I asked the AI about it and this was the explanation:
Your change will work, but pnpm test:ci is arguably better for CircleCI. Here's the difference:
| Script | Command |
|---|---|
| test:coverage | jest --maxWorkers=50% --coverage |
| test:ci | jest --maxWorkers=2 --coverage |
Both generate coverage reports (needed for the codecov upload), but:
test:ciuses a fixed 2 workers - more predictable/stable in CI containers with variable resourcestest:coverageuses 50% of available CPUs - good for local dev, but can be unpredictable or resource-heavy in CI
docs/BUILD_OPTIMIZATION_PLAN.md
Outdated
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.
Please drop the file.
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.
fixed
986f007 to
3ab8952
Compare
|
I just rebased this on latest |
3ab8952 to
adaec73
Compare
|
@Archaeopteryx I wanted to check if there's anything else you need from me on this PR. I have time to work on it today, if needed. |
Notes from Cam
yarn.lockremoval andpnpm-lock.yamladdition. Many of the files changed are just removing the hot reload.Migrate from Webpack to RSPack and Yarn to pnpm
Overview
This PR modernizes the Treeherder build infrastructure by:
Why RSPack?
RSPack is a high-performance JavaScript bundler written in Rust that offers:
Why pnpm?
pnpm offers several advantages over Yarn:
Changes Made
Build System Migration
webpack.config.js→rspack.config.jspackage.jsonrspackinstead ofwebpack.npmrc.yarnrcyarn.lock→pnpm-lock.yamlRSPack Configuration Highlights
builtin:swc-loaderfor faster JS/JSX transpilation (faster than Babel)@rspack/plugin-react-refreshfor HMR (replacesreact-hot-loader)@importdeprecation warnings (Bootstrap 5 still uses@import)Dependency Cleanup
Removed from dependencies (no longer needed)
react-hot-loader- Replaced by RSPack's built-in React RefreshRemoved from devDependencies
@babel/plugin-proposal-class-properties- Deprecated, built into @babel/preset-env@babel/plugin-syntax-dynamic-import- Built into @babel/preset-envpath- Built-in Node.js module, unnecessary as a dependencyMoved to devDependencies (were incorrectly in dependencies)
eslint-formatter-codeframe- Only used by ESLintglobals- Only used by ESLint configredux-mock-store- Only used in testsCode Changes
Removed
react-hot-loaderusage from 8 files:ui/App.jsxui/job-view/App.jsxui/login-callback/LoginCallback.jsxui/logviewer/App.jsxui/push-health/App.jsxui/intermittent-failures/App.jsxui/userguide/App.jsxui/perfherder/App.jsxConfiguration Updates
babel.config.json.prettierignorevenv/,node_modules/,pnpm-lock.yaml.circleci/config.ymldocker/Dockerfiledocs/*.mdTesting
All scripts pass successfully:
pnpm buildpnpm build:devpnpm lintpnpm testpnpm startpnpm format:checkpnpm markdownlintBuild Performance Comparison
While exact benchmarks depend on hardware, RSPack typically provides:
Migration Guide for Developers
For local development
Common commands (same as before, just use pnpm)
Package Manager Enforcement
This PR adds guards to ensure developers use pnpm instead of yarn or npm.
How It Works
1.
packageManagerfield inpackage.jsonThis field:
2.
preinstallscriptThis script:
packageManagerWhat Developers Will See
If they try to use Yarn:
If they try to use npm:
Migration for Developers
Breaking Changes
None. The migration is transparent to end users and maintains all existing functionality.
Remaining Warnings (informational only)
util._extenddeprecation from a transitive dependency. Does not affect functionality.Future Improvements (out of scope for this PR)
momenttodayjsfor better tree-shakingreact-router-domto v6