feat(core): replace esbuild with rolldown for mutator bundling#3628
feat(core): replace esbuild with rolldown for mutator bundling#3628thesebas wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces ChangesRolldown migration and new external/alias test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@anymaniax and @snebjorn since you guys are the build masters please give your thoughts! |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thesebas
force-pushed
the
rolldown
branch
2 times, most recently
from
69df9a7 to
4b6850b
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces esbuild with rolldown for runtime bundling inside getMutatorInfo() (used to bundle user-supplied mutator files before parsing their AST), aiming to reduce CLI cold-start overhead while preserving the existing public API.
Changes:
- Swapped the runtime bundler implementation in
bundleFile()fromesbuildtorolldown, adding properbundle.close()cleanup. - Added alias resolution and improved external handling (wildcard patterns + package subpath externalization) with new test coverage and fixtures.
- Updated
packages/coredependencies to removeesbuildand addrolldown.
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/generators/mutator-info.ts | Replaces esbuild bundling with rolldown, adds external-pattern matching and alias-aware externalization, and ensures bundle cleanup. |
| packages/core/src/generators/mutator-info.test.ts | Adds new tests covering alias resolution and expanded external semantics. |
| packages/core/src/generators/tests/mutator-test-files/external-wildcard-tests/styles.d.ts | Adds TS module declaration for *.scss fixture usage. |
| packages/core/src/generators/tests/mutator-test-files/external-wildcard-tests/mutation.ts | Adds fixture importing ./styles.scss to validate wildcard externals. |
| packages/core/src/generators/tests/mutator-test-files/external-package-subpath-tests/scoped-mutation.ts | Adds fixture importing a scoped package subpath to validate subpath externalization. |
| packages/core/src/generators/tests/mutator-test-files/external-package-subpath-tests/mutation.ts | Adds fixture importing an unscoped package subpath to validate subpath externalization. |
| packages/core/src/generators/tests/mutator-test-files/external-package-subpath-tests/modules.d.ts | Adds TS module declarations for external-package subpath fixtures. |
| packages/core/src/generators/tests/mutator-test-files/dynamic-import-tests/dynamic-import-named-export.ts | Updates fixture comments to be bundler-agnostic. |
| packages/core/src/generators/tests/mutator-test-files/call-expression-tests/factory-default-export.ts | Updates fixture comments to be bundler-agnostic. |
| packages/core/src/generators/tests/mutator-test-files/alias-resolution-tests/mutator-alias.d.ts | Adds TS module declaration for alias-resolution fixture. |
| packages/core/src/generators/tests/mutator-test-files/alias-resolution-tests/mutation.ts | Adds alias-resolution fixture that re-exports from the alias. |
| packages/core/package.json | Removes esbuild, adds rolldown. |
| bun.lock | Updates lockfile to reflect dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transform: { | ||
| target: compilerOptions?.target ?? 'es2015', | ||
| }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/generators/mutator-info.ts (1)
79-81: ⚡ Quick winPrecompute wildcard matchers once per
externaloption.
matchesExternalcreates a newRegExpper resolver call, and this path runs for every import resolution. Prebuilding predicates once ingetExternalOptionwill reduce repeated allocations in a performance-sensitive path.♻️ Suggested refactor
function getExternalOption( entry: string, external?: string[], alias?: Record<string, string>, ): ExternalOption { if (external) { - return (id) => external.some((pattern) => matchesExternal(pattern, id)); + const matchers = external.map((pattern) => { + if (isPackagePattern(pattern)) { + return (id: string) => id === pattern || id.startsWith(`${pattern}/`); + } + + if (!pattern.includes('*')) { + return (id: string) => id === pattern; + } + + const regex = new RegExp( + `^${pattern + .replaceAll(/[$()+./?[\\\]^{|}]/g, String.raw`\$&`) + .replaceAll('*', '.*')}$`, + ); + return (id: string) => regex.test(id); + }); + + return (id) => matchers.some((match) => match(id)); } @@ -function matchesExternal(pattern: string, id: string): boolean { +function matchesExternal(pattern: string, id: string): boolean {Also applies to: 100-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/generators/mutator-info.ts` around lines 79 - 81, The return function in the external options handler is calling matchesExternal inside the some() callback for every import resolution, causing new RegExp objects to be created repeatedly. Move the regex pattern compilation outside this loop by precomputing the matchers once when the external option is initially processed in getExternalOption, then use the precomputed matchers in the returned function instead of calling matchesExternal on each resolver call. This will eliminate the repeated RegExp allocations in this performance-critical path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/generators/mutator-info.ts`:
- Around line 79-81: The return function in the external options handler is
calling matchesExternal inside the some() callback for every import resolution,
causing new RegExp objects to be created repeatedly. Move the regex pattern
compilation outside this loop by precomputing the matchers once when the
external option is initially processed in getExternalOption, then use the
precomputed matchers in the returned function instead of calling matchesExternal
on each resolver call. This will eliminate the repeated RegExp allocations in
this performance-critical path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fd00b9d-7234-4a35-8dbc-43f2b6ac86c6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
packages/core/package.jsonpackages/core/src/generators/__tests__/mutator-test-files/alias-resolution-tests/mutation.tspackages/core/src/generators/__tests__/mutator-test-files/alias-resolution-tests/mutator-alias.d.tspackages/core/src/generators/__tests__/mutator-test-files/call-expression-tests/factory-default-export.tspackages/core/src/generators/__tests__/mutator-test-files/dynamic-import-tests/dynamic-import-named-export.tspackages/core/src/generators/__tests__/mutator-test-files/external-package-subpath-tests/modules.d.tspackages/core/src/generators/__tests__/mutator-test-files/external-package-subpath-tests/mutation.tspackages/core/src/generators/__tests__/mutator-test-files/external-package-subpath-tests/scoped-mutation.tspackages/core/src/generators/__tests__/mutator-test-files/external-wildcard-tests/mutation.tspackages/core/src/generators/__tests__/mutator-test-files/external-wildcard-tests/styles.d.tspackages/core/src/generators/mutator-info.test.tspackages/core/src/generators/mutator-info.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/generators/mutator-info.ts (1)
87-102: 💤 Low valueReDoS risk is minimal but consider documenting the assumption.
The static analysis flagged a potential ReDoS (CWE-1333) due to regex construction from variable input. However, the risk is low here because:
- The
externalpatterns come from developer-controlled build configuration, not untrusted user input- Patterns are typically simple globs like
*.scssorlodash/*- Module specifier strings being tested are short
The escaping covers all necessary regex special characters. No change is strictly required, but consider adding a brief code comment noting that patterns are assumed to be developer-configured build options.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/generators/mutator-info.ts` around lines 87 - 102, Add a brief code comment above the matchers variable assignment in the function that constructs the external pattern matchers, documenting the security assumption that the external patterns come from developer-controlled build configuration rather than untrusted user input. This will help future maintainers understand the context of the regex construction and why the ReDoS risk is acceptable in this specific case.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/generators/mutator-info.ts`:
- Around line 87-102: Add a brief code comment above the matchers variable
assignment in the function that constructs the external pattern matchers,
documenting the security assumption that the external patterns come from
developer-controlled build configuration rather than untrusted user input. This
will help future maintainers understand the context of the regex construction
and why the ReDoS risk is acceptable in this specific case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0de1440-13a8-4971-9e8f-e4268db7573b
📒 Files selected for processing (1)
packages/core/src/generators/mutator-info.ts
|
Build still failing. |
|
Thanks for the PR and the benchmarks @thesebas 🙏 My main concern is the risk of side effects. Swapping the bundler also swaps the downleveling engine (oxc vs esbuild), and our mutator arity detection depends on how async/modern syntax gets lowered for a given tsconfig target. Different lowering can produce different generated client output in subtle ways, and for a codegen tool that kind of silent change is risky and hard to fully cover with tests. On top of that it adds a lot of dependency footprint — a noticeably bigger bundler shipped to everyone — while the gain is small and only benefits users with custom mutators (~120 ms of cold start). So for me the trade-off isn't really worth it right now. The new tests and the bundle.close() cleanup are genuinely nice though — would you be open to splitting those into a separate PR on the current esbuild path? @snebjorn @melloware wdyt? |
|
I agree with your assessment the trade offs aren't big enough |
Summary
Replace
esbuildwithrolldownas the runtime bundler used bygetMutatorInfo()to parse user-supplied mutator files. This is the only place in the codebase that uses a bundler at runtime — the project's own build pipeline (vp pack) already uses rolldown via vite-plus.Motivation
getMutatorInfo()invokes a bundler to bundle the user's mutator source file before parsing its AST to determine the mutator's arity (number of parameters). Everyorvalinvocation that uses custom mutators pays this cold-start cost. Rolldown's WASM-based native binding starts 11× faster than esbuild's Go binary, making CLI usage noticeably snappier.Performance Comparison
All measurements from
bun test mutator-infoinpackages/core/:Dependency Footprint
@oxc-project/types,@rolldown/pluginutilsRolldown is 2× larger on disk but has fewer platform-specific variants (15 vs 26). Rolldown also removes esbuild's native postinstall script (
install.js) — its native bindings use standard npm optionalDependencies instead.Dependency Range
Uses
"^1.0.0"— the minimum stable version with all required features. Rolldown is already a transitive dependency of the workspace (via@angular/buildat1.0.0-rc.4), so specifying^1.0.0lets the package manager deduplicate naturally while ensuring the features we need are available.Functionality Verification
es2015+ target for transformed output viatransform.target.getMutatorInfo()public API is unchanged — consumers are unaffected.1.0.0and1.1.2.New Capabilities
The rolldown implementation adds explicit support for features that esbuild handled implicitly:
@mutator-aliasimports via thealiasoption*.scssand similar glob patterns@scope/pkg/subpathgivenexternal: ["@scope/pkg"]These are covered by 5 new test cases in
mutator-info.test.ts.Rollup-compatible API
Rolldown's API is rollup-compatible, returning a
bundleinstance with.generate()/.write()/.close(). This enables:try/finally(the esbuild version never called any close-equivalent)Summary by CodeRabbit
Tests
*.scss.import()regression notes while keeping the same verification behavior.Chores