Skip to content

feat(build): support tsdown for building#11

Merged
kfirstri merged 2 commits intomainfrom
build-with-tsdown
Jan 8, 2026
Merged

feat(build): support tsdown for building#11
kfirstri merged 2 commits intomainfrom
build-with-tsdown

Conversation

@kfirstri
Copy link
Contributor

@kfirstri kfirstri commented Jan 8, 2026

Summary

Build project with tsdown instead of just using tsc

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/cli@0.0.1-pr.11.72924bc

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.1-pr.11.72924bc"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/cli@0.0.1-pr.11.72924bc"
  }
}

Preview published to npm registry — try new features instantly!

@claude
Copy link

claude bot commented Jan 8, 2026

PR Review: Build with tsdown

Summary

This PR successfully migrates the build system from tsc + tsc-alias to tsdown, a modern bundler powered by Rolldown (Rust-based Rollup successor). The changes are well-structured and align with the project's goals of modernizing the build pipeline.

✅ Strengths

1. Clean Build Configuration

  • The tsdown.config.ts is minimal and well-configured
  • Properly excludes node_modules from bundling (skipNodeModulesBundle: true)
  • Correct output structure with single ESM bundle

2. Improved CI/CD

  • All GitHub Actions workflows now use .node-version file instead of hardcoded versions
  • Added dedicated typecheck workflow - excellent separation of concerns
  • Consistent Node.js version management across all workflows

3. Documentation Updates

  • AGENTS.md properly updated with new tooling information
  • Clear distinction between build (tsdown) and typecheck (tsc --noEmit)
  • Added Node.js version requirements

4. Package Management

  • Correctly replaced tsc-alias with tsdown in dependencies
  • Package.json scripts are clean and well-organized
  • Build output configured correctly (main, bin, exports)

🔍 Observations & Recommendations

1. TypeScript Configuration (Minor)

The tsconfig.json has "noEmit": true which is correct for the new setup. However, consider adding a comment to clarify this is intentional:

// tsconfig.json
{
  "compilerOptions": {
    "noEmit": true, // Type checking only - tsdown handles compilation
    // ... rest
  }
}

2. Path Alias Handling (Important)

The PR correctly keeps the @core/* path alias in tsconfig.json. However, I notice that tsdown.config.ts doesn't explicitly configure path alias resolution. Please verify:

  • Does tsdown automatically resolve TypeScript path aliases from tsconfig.json?
  • Test that imports like import { readProjectConfig } from "@core/config/project.js" resolve correctly in the built output

If path resolution fails, you may need to add explicit path configuration to tsdown.config.ts:

export default defineConfig({
  // ... existing config
  resolve: {
    alias: {
      '@core': './src/core'
    }
  }
});

3. Build Verification

Before merging, please verify:

  • Run npm run build successfully
  • Check that dist/cli/index.js is created and executable
  • Verify the bundle size is reasonable
  • Test that node dist/cli/index.js --help works correctly
  • Confirm that the built CLI can be executed via the bin entry

4. Testing (Critical)

The existing test at tests/core/project.test.ts uses direct imports from source:

import { readProjectConfig } from "../../src/core/config/project.js";

This is fine for development, but consider:

  • Are there integration tests that verify the built output?
  • Should CI run tests against both source (npm run dev) and built output?

5. Bundle Analysis

Since you're now using a bundler:

  • Consider adding a script to inspect bundle size: "bundle-size": "ls -lh dist/cli/index.js"
  • Verify that unused code is tree-shaken appropriately
  • Check that all necessary dependencies are included in the bundle

6. Production Considerations

  • The skipNodeModulesBundle: true setting means dependencies are not bundled. Verify this is intentional and that all dependencies are properly listed in dependencies (not devDependencies)
  • Confirm that consumers installing via npm will have all required dependencies available

7. Missing Changelog Entry

Consider adding a changelog or release notes entry documenting this significant build system change for future reference.

🐛 Potential Issues

Issue 1: Missing Build Verification in CI

None of the CI workflows actually run npm run build to verify the build succeeds. Consider adding:

# .github/workflows/build.yml
name: Build

on:
  push:
    branches: [main]
  pull_request:
    branches: [main]

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-node@v4
        with:
          node-version-file: ".node-version"
          cache: "npm"
      - run: npm ci
      - run: npm run build
      - run: node dist/cli/index.js --version  # Smoke test

Issue 2: Package.json Licenses

I notice many dependencies in package-lock.json now have explicit MIT licenses. This is good for compliance, but verify:

  • All licenses are compatible with your project's ISC license
  • No unexpected license changes were introduced

🔒 Security Considerations

  • ✅ No security vulnerabilities identified in the new dependencies
  • tsdown is a reputable package from the Rolldown ecosystem
  • ✅ No sensitive data or credentials in configuration files

⚡ Performance Considerations

  • Build Speed: tsdown (powered by Rolldown/Rust) should provide significantly faster builds than tsc + tsc-alias
  • Bundle Size: Single file output may be larger than multiple files but simplifies distribution
  • Runtime Performance: No runtime impact - still using Node.js ESM

📝 Code Quality

  • ✅ Follows project conventions from AGENTS.md
  • ✅ Maintains ES modules with .js extensions in imports
  • ✅ Consistent with existing project structure
  • ✅ No breaking changes to public API

🧪 Test Coverage

  • ✅ Existing tests should still work with the new build system
  • ⚠️ Missing: Tests for the build output itself
  • ⚠️ Missing: CI verification that build completes successfully

Summary of Recommendations

Before Merging:

  1. ✅ Add a CI workflow to verify npm run build succeeds
  2. ✅ Test the built output manually: npm run build && node dist/cli/index.js
  3. ✅ Verify path alias resolution works in the bundle
  4. ⚠️ Confirm bundle size is acceptable
  5. ⚠️ Add smoke tests for the built CLI

Nice to Have:

  • Add bundle size reporting
  • Document the migration in a changelog
  • Add integration tests for built output

Conclusion

This is a solid PR that modernizes the build system effectively. The migration to tsdown is well-executed with good attention to detail in CI/CD updates and documentation. The main concern is the lack of build verification in CI and potential path alias resolution issues.

Recommendation: Request changes to add build verification to CI before merging. Once that's added and tested, this is ready to merge.


Overall Rating: 8/10 - Well-executed migration with minor testing gaps

Great work on modernizing the build pipeline! 🚀

@claude
Copy link

claude bot commented Jan 8, 2026

Updated PR Review: Build with tsdown

Overview

This PR successfully migrates the build system from tsc + tsc-alias to tsdown, a modern bundler powered by Rolldown. The migration is clean and well-documented.


✅ Strengths

1. Excellent Build Configuration

The tsdown.config.mjs is minimal and properly configured with single entry point, ESM format, and auto-clean.

2. Proper TypeScript Separation

Excellent separation: Build (tsdown) vs Typecheck (tsc --noEmit). This is modern best practice.

3. CI/CD Improvements

  • All workflows now use .node-version file (excellent consistency)
  • New dedicated typecheck.yml workflow
  • Proper Node.js version pinning (20.19.0)

4. Documentation Quality

AGENTS.md comprehensively updated with build tools, workflow, and requirements.

5. Package.json Configuration

All build-related changes are correct.


🔍 Key Recommendations

1. Path Alias Resolution (Verify)

The tsconfig includes path aliases (@core/*). Please verify tsdown correctly resolves these in the bundle. If not, may need to add alias config to tsdown.config.mjs.

2. Missing Build Verification in CI ⚠️

Critical Issue: None of the CI workflows run npm run build to verify the build succeeds.

Recommendation: Add a build.yml workflow that:

  • Runs npm run build
  • Verifies dist/cli/index.js exists
  • Smoke tests with node dist/cli/index.js --version

This is critical to catch build failures before merge.

3. packageVersion.ts Path Resolution

File src/cli/utils/packageVersion.ts:10 uses join(__dirname, "..", "..", "package.json")

Concern: After bundling, will __dirname resolution work correctly? Test that getPackageVersion() works in built output.

4. Dependency Bundling

Config doesn't specify external or bundling options. Please verify:

  • Are dependencies bundled or kept external?
  • What's the bundle size after building?
  • For CLI tools, typically want to bundle dependencies.

🐛 Potential Issues

Issue 1: No Build CI ⚠️

Critical - must add before merging.

Issue 2: Test Coverage Gap

Tests import from source (../../src/core/config/project.js). No tests verify the built output works. Consider adding integration tests for the built CLI.

Issue 3: .gitignore Check

Verify .gitignore includes dist/.


🔒 Security & Performance

  • ✅ No security vulnerabilities identified
  • tsdown is reputable (Rolldown ecosystem)
  • ✅ No credentials in configs
  • ✅ Expected faster builds (Rust-based)

Action: Benchmark build time and check bundle size.


📋 Pre-Merge Checklist

Critical:

  • Add build verification to CI
  • Test: npm run build && node dist/cli/index.js --help
  • Verify path aliases work in bundle
  • Test getPackageVersion() in built output
  • Check bundle size

Recommended:

  • Add integration tests for built CLI
  • Verify .gitignore includes dist/
  • Test the preview package

🎯 Conclusion

Well-executed migration with high code quality and thorough documentation.

Main concerns:

  1. ⚠️ Missing build verification in CI (must fix)
  2. ⚠️ No verification built output works (should test)
  3. ℹ️ Path alias resolution needs verification

Recommendation: Request changes to add build CI, then approve after testing.


Overall Rating: 8.5/10 - Excellent migration with minor CI/testing gaps

Great work on modernizing the build pipeline! 🚀

@kfirstri kfirstri merged commit 39bd55e into main Jan 8, 2026
5 checks passed
@kfirstri kfirstri deleted the build-with-tsdown branch January 8, 2026 10:25
@github-project-automation github-project-automation bot moved this from Backlog to Done in CLI Development Jan 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant