-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: Enable Meteor's *Modern Build Stack* #37614
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: develop
Are you sure you want to change the base?
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis pull request modernizes the Meteor build toolchain by updating JSX runtime configuration, introducing SWC as a compilation tool, and applying targeted patches to the typia dependency across multiple packages. Configuration changes span Babel, platform settings, and package metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core-typings/package.json (1)
6-26: Build surface for@rocket.chat/core-typingslooks good; reconsider always-failingtestscriptPositives:
- Adding
main,typings, andfiles: ["/dist"]plus thetsc/ts-patch/typia patchpipeline makes this package behave like a standard compiled TS workspace module.- Dependencies on
@rocket.chat/icons,@rocket.chat/message-parser,@rocket.chat/ui-kit, and the patchedtypiaare consistent with the broader monorepo setup.One caution:
- The
testscript currently runsecho "no tests" && exit 1, which will always fail. If your roottestcommand or CI uses a workspace-widetestscript runner, this package will cause the entire test job to fail even when “no tests” is expected.If this package isn’t yet supposed to have tests, consider either:
- Returning success instead (e.g.
echo "no tests" && exit 0), or- Omitting the
testscript and relying on the root tooling to skip packages without tests.That keeps the door open for adding real tests later without surprising CI failures in the meantime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.yarn/patches/typia-npm-9.7.2-5c5d9c80b4.patchis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
apps/meteor/.babelrc(1 hunks)apps/meteor/.meteor/platforms(1 hunks)apps/meteor/.swcrc(1 hunks)apps/meteor/package.json(2 hunks)packages/core-typings/package.json(1 hunks)packages/ui-kit/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (1/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (1/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.2 (4/4)
- GitHub Check: 🔨 Test API (EE) / MongoDB 5.0 (1/1)
🔇 Additional comments (6)
apps/meteor/.swcrc (1)
1-9: SWC React/runtime config looks correct and aligns with Meteor’s SWC docs
jsc.target: "es2022"plustransform.react.runtime: "automatic"matches the documented way to enable the automatic React JSX runtime via.swcrcfor Meteor’s modern SWC-based stack.(docs.meteor.com) This should keep Babel and SWC behavior consistent.Just confirm this target is compatible with the browsers you intend to support (given
browserslist: ["defaults"]) and adjust if you still need older engines.apps/meteor/.meteor/platforms (1)
1-3: Platforms configuration matches Meteor’s modern build stack guidanceHaving:
serverbrowsermodernin
.meteor/platformsmatches the example Meteor gives for excluding legacy bundles in production when using the modern build stack.(fossies.org) This looks correct; just ensure you’re comfortable dropping legacy browser bundles in production.apps/meteor/.babelrc (1)
4-4: Using@babel/preset-reactwithruntime: "automatic"is a clean JSX setupSwitching JSX handling to
@babel/preset-reactwithruntime: "automatic"is idiomatic and avoids the need for the old JSX transform plugin. It aligns Babel’s JSX behavior with your SWCreact.runtime = "automatic"config, reducing divergence between toolchains.No issues from a config perspective; just verify your tests/Storybook don’t rely on the classic JSX runtime assumptions.
packages/ui-kit/package.json (1)
34-34: Typia patched dependency is consistent with the workspace patch strategyPointing
typiato the Yarnpatch:spec keeps this package aligned with the rest of the workspace’s patched typia usage and with thets-patch/typia patchbuild step already defined in scripts.Please just double‑check that:
- The patch file
typia-npm-9.7.2-5c5d9c80b4.patchis present under the expected.yarn/patchespath in the repo, and- Your Yarn version in CI/dev fully supports this
patch:URL form.apps/meteor/package.json (2)
454-454: App-level typia patch keeps runtime dependency in sync with patched toolchainUpdating the app’s
typiadependency to the same Yarnpatch:spec used in other packages keeps the runtime dependency aligned with your patched compiler/tooling setup. That should avoid version skew between wheretypiais used to generate types and where it’s used at runtime.As with the other packages, just confirm the patch file is present in
.yarn/patchesand that CI uses the same Yarn version that generated this spec.
467-477: Meteor modern build flags and browserslist changes look coherent; confirmdisableLegacyBuildsemantics
"meteor": { "modern": true, ... }is exactly how Meteor recommends enabling the modern build stack (SWC transpiler, modern bundler/dev‑server behavior).(docs.meteor.com)- Combined with
.meteor/platformsincludingmodern, this should drop legacy browser bundles in production while using the modern stack during dev.Two things worth double‑checking:
disableLegacyBuild: this flag isn’t clearly surfaced in the public Meteor docs yet. Please confirm your target Meteor version actually recognizes it (and that it does what you expect) rather than silently ignoring it.browserslist: ["defaults"]: this is a sensible general target, but it may differ slightly from your old"last 2 versions", "Firefox ESR"set. Confirm this matches the actual browser support policy you want for the app.If both match your intentions, the overall config is consistent with Meteor’s modern stack guidance.
There is a pending issue with coverage
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37614 +/- ##
===========================================
+ Coverage 70.80% 71.93% +1.13%
===========================================
Files 3159 1474 -1685
Lines 109401 77299 -32102
Branches 19675 11256 -8419
===========================================
- Hits 77456 55606 -21850
+ Misses 29917 21237 -8680
+ Partials 2028 456 -1572
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
https://rocketchat.atlassian.net/browse/ARCH-1911
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.