-
Notifications
You must be signed in to change notification settings - Fork 19
chore: implement bundle script #219
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
Conversation
|
Warning Rate limit exceeded@sampaiodiego has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughIntroduces a Bun-based bundling workflow for the federation SDK, adds a types-only tsconfig, updates package scripts, adjusts federation-sdk entry points to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant B as bundle.ts
participant Bun as Bun.build
participant FS as File System
participant TSC as tsc (emitDeclarationOnly)
Dev->>B: Run "bun run bundle.ts"
B->>FS: Read package.jsons (core, crypto, federation-sdk, room)
B->>Bun: Build entry: src/index.ts (CJS, minify, sourcemap, externals)
Bun-->>FS: Write dist artifacts
B->>FS: Prune workspace deps -> write output package.json
B->>TSC: Generate .d.ts via tsconfig.sdk.types.json
TSC-->>FS: Emit declarations
B-->>Dev: Log "Bundle complete!"
Note over B,Bun: Bun-based bundling replaces prior esbuild flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
=======================================
Coverage 81.07% 81.07%
=======================================
Files 63 63
Lines 4719 4719
=======================================
Hits 3826 3826
Misses 893 893 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a6c223a to
75fbdab
Compare
75fbdab to
ae52adb
Compare
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: 2
🧹 Nitpick comments (2)
package.json (2)
31-31: Update Bun version to latest stable release.The current packageManager specifies "bun@1.1.10", but the latest stable release is v1.2.22. Version 1.2.21 includes significant improvements like MySQL/SQLite support, YAML parsing, reduced memory usage, and bug fixes addressing 69 issues.
Apply this diff to update to the latest version:
-"packageManager": "bun@1.1.10", +"packageManager": "bun@1.2.22",
54-54: Consider extracting bundle:watch configuration.The
bundle:watchcommand is quite long with many inline options. Consider moving the configuration to a dedicated config file or script for better maintainability.Create a
bundle.config.jsor add abun.config.jsfile:export default { entrypoints: ['./packages/federation-sdk/src/index.ts'], outdir: './federation-bundle/dist', target: 'node', format: 'cjs', external: [ 'pino', 'mongodb', 'zod', 'pino-pretty', '@rocket.chat/emitter', 'reflect-metadata', 'tsyringe', 'tweetnacl' ], production: true, sourcemap: 'inline', watch: true };Then update the script:
-"bundle:watch": "bun build --watch ./packages/federation-sdk/src/index.ts --outdir ./federation-bundle/dist --target node --format=cjs -e pino -e mongodb -e zod -e pino-pretty -e @rocket.chat/emitter -e reflect-metadata -e tsyringe -e tweetnacl --production --sourcemap=inline" +"bundle:watch": "bun build --config bun.config.js"
📜 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 selected for processing (5)
.gitignore(1 hunks)bundle.ts(1 hunks)package.json(1 hunks)packages/federation-sdk/package.json(1 hunks)tsconfig.sdk.types.json(1 hunks)
🔇 Additional comments (5)
.gitignore (2)
187-187: LGTM! Appropriate ignore pattern for bundle output.The federation-bundle/ ignore pattern aligns with the latest Bun v1.2.22 bundling capabilities and matches the AI summary indicating this supports the new bundling workflow that outputs under federation-bundle/.
185-185: Ignore suggestion to change “-link” to “link”;.gitignorealready listslinkand no-linkpatterns exist.Likely an incorrect or invalid review comment.
package.json (2)
52-52: LGTM! Simplified TypeScript compilation.The change from
bunx tsctotsc --noEmitis more straightforward for type checking without generating output files, which aligns with the new bundle-focused workflow.
53-53: bundle.ts presence verified The file exists with the expected build logic, so thebundle:sdkscript is valid.packages/federation-sdk/package.json (1)
5-5: No action needed: build emits dist/index.js as expected – TypeScript’soutDir: "./dist"andsrc/index.tsensuredist/index.jsis generated; nobundle.jsreferences remain.
| function getAllDependencies() { | ||
| const packages = ['core', 'crypto', 'federation-sdk', 'room']; | ||
|
|
||
| const allDependencies = new Set<string>(); | ||
|
|
||
| for (const pkg of packages) { | ||
| const packageJson = require(`./packages/${pkg}/package.json`); | ||
|
|
||
| const dependencies = packageJson.dependencies | ||
| ? Object.keys(packageJson.dependencies) | ||
| : []; | ||
|
|
||
| for (const dep of dependencies) { | ||
| allDependencies.add(dep); | ||
| } | ||
| } | ||
|
|
||
| return Array.from(allDependencies); | ||
| } | ||
|
|
||
| async function main() { | ||
| await $`rm -rf ${outputDir}/dist`; | ||
| await $`mkdir -p ${outputDir}/dist`; | ||
|
|
||
| const dependencies = getAllDependencies(); | ||
|
|
||
| await Bun.build({ | ||
| entrypoints: [`${inputDir}/src/index.ts`], | ||
| outdir: `${outputDir}/dist`, | ||
| target: 'node', | ||
| format: 'cjs', | ||
| external: dependencies, | ||
| env: 'disable', | ||
| define: { | ||
| 'process.env.NODE_ENV': '"production"', | ||
| }, | ||
| minify: true, | ||
| sourcemap: true, | ||
| }); |
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.
Fix require usage under Bun’s ESM runtime
Line 13: executing this file via bun run bundle.ts treats it as ESM, so require is undefined and the script aborts before bundling. Swap to Bun’s file API and make the helper async so JSON loading works under Bun.
-// get dependencies from all packages
-function getAllDependencies() {
+// get dependencies from all packages
+async function getAllDependencies() {
const packages = ['core', 'crypto', 'federation-sdk', 'room'];
const allDependencies = new Set<string>();
for (const pkg of packages) {
- const packageJson = require(`./packages/${pkg}/package.json`);
+ const packageJson = JSON.parse(
+ await Bun.file(`./packages/${pkg}/package.json`).text(),
+ );
const dependencies = packageJson.dependencies
? Object.keys(packageJson.dependencies)
: [];
for (const dep of dependencies) {
allDependencies.add(dep);
}
}
return Array.from(allDependencies);
}
async function main() {
await $`rm -rf ${outputDir}/dist`;
await $`mkdir -p ${outputDir}/dist`;
- const dependencies = getAllDependencies();
+ const dependencies = await getAllDependencies();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getAllDependencies() { | |
| const packages = ['core', 'crypto', 'federation-sdk', 'room']; | |
| const allDependencies = new Set<string>(); | |
| for (const pkg of packages) { | |
| const packageJson = require(`./packages/${pkg}/package.json`); | |
| const dependencies = packageJson.dependencies | |
| ? Object.keys(packageJson.dependencies) | |
| : []; | |
| for (const dep of dependencies) { | |
| allDependencies.add(dep); | |
| } | |
| } | |
| return Array.from(allDependencies); | |
| } | |
| async function main() { | |
| await $`rm -rf ${outputDir}/dist`; | |
| await $`mkdir -p ${outputDir}/dist`; | |
| const dependencies = getAllDependencies(); | |
| await Bun.build({ | |
| entrypoints: [`${inputDir}/src/index.ts`], | |
| outdir: `${outputDir}/dist`, | |
| target: 'node', | |
| format: 'cjs', | |
| external: dependencies, | |
| env: 'disable', | |
| define: { | |
| 'process.env.NODE_ENV': '"production"', | |
| }, | |
| minify: true, | |
| sourcemap: true, | |
| }); | |
| // get dependencies from all packages | |
| async function getAllDependencies() { | |
| const packages = ['core', 'crypto', 'federation-sdk', 'room']; | |
| const allDependencies = new Set<string>(); | |
| for (const pkg of packages) { | |
| const packageJson = JSON.parse( | |
| await Bun.file(`./packages/${pkg}/package.json`).text(), | |
| ); | |
| const dependencies = packageJson.dependencies | |
| ? Object.keys(packageJson.dependencies) | |
| : []; | |
| for (const dep of dependencies) { | |
| allDependencies.add(dep); | |
| } | |
| } | |
| return Array.from(allDependencies); | |
| } | |
| async function main() { | |
| await $`rm -rf ${outputDir}/dist`; | |
| await $`mkdir -p ${outputDir}/dist`; | |
| const dependencies = await getAllDependencies(); | |
| await Bun.build({ | |
| entrypoints: [`${inputDir}/src/index.ts`], | |
| outdir: `${outputDir}/dist`, | |
| target: 'node', | |
| format: 'cjs', | |
| external: dependencies, | |
| env: 'disable', | |
| define: { | |
| 'process.env.NODE_ENV': '"production"', | |
| }, | |
| minify: true, | |
| sourcemap: true, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In bundle.ts around lines 7 to 45, the helper uses require() which fails under
Bun's ESM runtime; make getAllDependencies async and replace require calls with
Bun.file(...).text() followed by JSON.parse (or Bun.file(...).json() if
available), iterate packageJson.dependencies the same way, and return the
dependency array; update main to await getAllDependencies() so bundling runs
with the resolved external list.
Summary by CodeRabbit
New Features
Chores