Skip to content

Bun build with watch#198

Merged
artemdemo merged 8 commits intomainfrom
bun-build
Feb 5, 2026
Merged

Bun build with watch#198
artemdemo merged 8 commits intomainfrom
bun-build

Conversation

@artemdemo
Copy link
Contributor

@artemdemo artemdemo commented Feb 5, 2026

Note

Description

Refactors the build process by extracting build logic into a standalone build.ts script in the new infra/ directory. This improves maintainability and enables a new watch mode that automatically rebuilds when source files change, significantly improving the development experience. The build output structure is also updated to better organize bundled CLI code (dist/cli/) and template files (dist/templates/).

Related Issue

None

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Other (please describe):

Changes Made

  • Created standalone infra/build.ts script with explicit Bun build configuration and colored console output
  • Added watch mode (--watch flag) that monitors ./src directory and automatically rebuilds on changes
  • Updated build output structure: dist/cli/index.js (bundled code) and dist/templates/ (template files)
  • Added build:watch npm script for development workflow
  • Updated bin/run.js to import from new dist/cli/index.js path
  • Fixed template path resolution in src/core/config.ts to match new structure (../templates instead of templates)
  • Updated test path in CLITestkit.ts to reference new bundled file location
  • Enhanced clean script to create dist directory after removal

Testing

  • I have tested these changes locally
  • I have added/updated tests as needed
  • All tests pass (npm test)

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have updated CLAUDE.md if I made architectural changes

Additional Notes

The watch mode uses Node.js fs.watch with recursive monitoring since Bun.build doesn't provide a programmatic watch API. Build output includes colored console messages using chalk for better visibility (green checkmarks for success, red for errors, cyan for file paths, yellow for watch mode status).

Note: This changes the internal build output structure but does not affect the public API - the npm package still works identically for end users since bin/run.js correctly imports from the new location.


🤖 Generated by Claude | 2026-02-05 21:42 UTC

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2026

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/cli@0.0.28-pr.198.1b295ba

Prefer not to change any import paths? Install using npm alias so your code still imports base44:

npm i "base44@npm:@base44-preview/cli@0.0.28-pr.198.1b295ba"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/cli@0.0.28-pr.198.1b295ba"
  }
}

Preview published to npm registry — try new features instantly!

@artemdemo artemdemo marked this pull request as ready for review February 5, 2026 09:15
@claude
Copy link

claude bot commented Feb 5, 2026

Code Review

Found 1 issue:


Missing initial build in watch mode

File: build.ts (line 20)

In watch mode, no initial build is performed. The runBuild() function is only called inside the watch callback, which means users must wait for a file change before the first build happens.

When running bun run build:watch, the templates are copied (line 17) but no JavaScript bundle is built until a source file is modified. This leaves the dist directory missing the main index.js output, causing the CLI to fail if run immediately.

Suggested fix:

Add an initial runBuild() call before setting up the watcher:

await cp("./templates", path.join(distDir, "templates"), { recursive: true });

if (args.includes("--watch")) {
  console.log("Watching for changes...");
  
  // Perform initial build
  const initialBuild = await runBuild();
  console.log(initialBuild.outputs.map((o) => o.path).join(", "));

  // Watch for changes
  for await (const _event of watch("./src", { recursive: true })) {
    const build = await runBuild();
    console.log(build.outputs.map((o) => o.path).join(", "));
  }
  
  await new Promise(() => {});
}

Context:

cli/build.ts

Lines 17 to 27 in 768e53e

await cp("./templates", path.join(distDir, "templates"), { recursive: true });
if (process.argv.includes("--watch")) {
console.log("\nWatching for changes...");
for (const dir of ["./src"]) {
watch(dir, { recursive: true }, async (event, filename) => {
const cliBuild = await runBuild();
console.log(cliBuild.outputs.map((o) => o.path).join(", "));
});
}

build.ts Outdated
if (process.argv.includes("--watch")) {
console.log("\nWatching for changes...");

for (const dir of ["./src"]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this? future proofing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, there will be another entry directory.
I need to build Deno code, so I prefer not to put it together with Node

build.ts Outdated
for (const dir of ["./src"]) {
watch(dir, { recursive: true }, async (event, filename) => {
const cliBuild = await runBuild();
console.log(cliBuild.outputs.map((o) => o.path).join(", "));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing 🙏

Copy link
Contributor

@kfirstri kfirstri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love that we're extracting it out

const runBuild = async () => {
const result = await Bun.build({
entrypoints: ["./src/cli/index.ts"],
outdir: "./dist/cli",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving pure cli code into dedicated folder in dist.
This way we can safely add another output in dist (like for additional entries) without concerning with files overriding each other.

);
};

await runBuild();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch mode should start with fully building the code, otherwise it will wait for the first change to happen.


// After bundling, import.meta.url points to dist/cli/index.js
// Templates are copied to dist/cli/templates/
// Templates are copied to dist/templates/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment was originally incorrect. templates were always in dist/templates/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one


export function getTemplatesDir(): string {
return join(__dirname, "templates");
return join(__dirname, "../templates");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since cli code is now in dist/cli - this path should change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we just need to make sure it's working fine in dev + in dist

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you say dev, what do you mean?
aren't templates used only for creation of the new app?
I tested it for creation and it works fine.

"dev": "./bin/dev.ts",
"start": "./bin/run.js",
"clean": "rm -rf dist",
"clean": "rm -rf dist && mkdir -p dist",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recreating dist folder, so I could copy templates folder before running bun build

@@ -1,5 +1,5 @@
#!/usr/bin/env node
import { runCLI } from "../dist/index.js";
import { runCLI } from "../dist/cli/index.js";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments about output changes below

@artemdemo
Copy link
Contributor Author

@claude review

@claude
Copy link

claude bot commented Feb 5, 2026

Claude finished @artemdemo's task in 1m 9s —— View job


Code Review

This is a solid refactoring that improves the build process organization and adds useful watch mode functionality. The code is well-structured and the changes are mostly straightforward. Here are my findings:


✅ Strengths

Good architectural decisions:

  • Extracting build logic into a dedicated TypeScript file is much cleaner than inline package.json commands
  • The watch mode implementation is straightforward and uses Node.js built-in fs.watch
  • The initial build before watch (infra/build.ts:43) correctly addresses the issue identified in Claude's earlier comment
  • Output structure change (dist/cli/ vs dist/) provides better organization for future expansion (like Deno builds)

Clean code:

  • Good use of chalk for colored output
  • Clear separation between watch and non-watch modes
  • Proper error handling with process.exit(1) on build failures

🔍 Issues Found

1. Inconsistent array iteration pattern (infra/build.ts:45)

for (const dir of ["./src"]) {
  watch(dir, { recursive: true }, changeHandler);
}

This iterates over an array with only one element. Either:

  • Remove the array if there will always be one directory: watch("./src", { recursive: true }, changeHandler);
  • Or if planning to add more directories, add a comment explaining the future-proofing (as mentioned in review comments)

Recommendation: If planning to watch multiple directories (like for Deno builds), add a comment. Otherwise, simplify to direct call.


2. Unused formatOutput function (infra/build.ts:24-26)

The function is defined but only used once in the non-watch branch. In the watch branch (line 39), you're manually formatting:

chalk.cyan(result.outputs[0]?.path ?? "")

Recommendation: Either use formatOutput consistently in both branches, or inline it if it's not providing value. Current inconsistency is confusing.


3. Missing template copy in build:watch (package.json:15)

The build:watch script copies templates once at startup:

"build:watch": "npm run clean && cp -r templates dist/ && bun run infra/build.ts --watch"

If templates are modified during watch mode, they won't be re-copied. This could lead to stale templates during development.

Recommendation: Either:

  • Document this limitation in the PR description or as a comment
  • Or add templates/ to the watch directories and handle template copying in the change handler
  • Or accept this as acceptable behavior (templates rarely change during rapid CLI iteration)

4. Test path hardcoding (tests/cli/testkit/CLITestkit.ts:12)

The path is correctly updated:

const DIST_INDEX_PATH = join(__dirname, "../../../dist/cli/index.js");

However, this creates a tight coupling to the directory structure. If the test directory moves or the output structure changes again, this breaks.

Recommendation: Consider importing from package root or using a constant. Not critical since this is internal test code, but worth noting.


🎯 Minor Observations

  1. Cross-platform compatibility (package.json:14-15)

    • The cp -r command in build scripts is Unix-only
    • The CLAUDE.md mentions "Use path module utilities, never hardcode separators" for cross-platform support
    • Consider using a Node script or fs-extra for copying in the future if Windows support is needed
  2. Watch error handling

    • The changeHandler doesn't catch errors from runBuild()
    • If a build fails during watch, the error is logged but the process continues (which is probably correct behavior)
    • Consider whether you want to surface build failures more prominently
  3. Process keep-alive pattern (infra/build.ts:50)

    await new Promise(() => {});

    This is a clever way to keep the process alive, though setInterval(() => {}, 1 << 30) is another common pattern. Current approach is fine.


📋 Summary

Verdict: ✅ Approve with minor suggestions

The PR achieves its stated goals and the code quality is good. The issues identified are minor and don't block merging:

The refactoring provides clear value and sets up a good foundation for future build configuration extensions.


| Branch: bun-build

package.json Outdated
"scripts": {
"build": "rm -rf dist && bun build src/cli/index.ts --outdir dist --target node --format esm --sourcemap=inline && cp -r templates dist/",
"build": "npm run clean && cp -r templates dist/ && bun run infra/build.ts",
"build:watch": "npm run clean && cp -r templates dist/ && bun run infra/build.ts --watch",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for the sake of being aligned, can we use bun run clean in both commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

kfirstri
kfirstri previously approved these changes Feb 5, 2026
Copy link
Contributor

@kfirstri kfirstri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine, just worried that this

export function getTemplatesDir(): string {
  return join(__dirname, "../templates");Collapse comment
}

might break stuff between dev/release version?

other than that - awesome 💪

@artemdemo artemdemo merged commit 2dbeda0 into main Feb 5, 2026
7 checks passed
@artemdemo artemdemo deleted the bun-build branch February 5, 2026 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants