chore: switch build to tsdown#2760
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
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. |
kettanaito
left a comment
kettanaito
left a comment
There was a problem hiding this comment.
This looks good for me! Thank you, @Andarist 👏
I've kicked off CodeRabbit to take a look at it, too. Everything's building/passing locally.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
config/plugins/rolldown/resolveCoreImportsPlugin.ts (1)
10-10: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent ESM detection vs.
forceFileExtensionsPlugin.This plugin derives
isEsmpurely fromchunk.fileName.endsWith(ESM_EXTENSION), whereasforceFileExtensionsPlugin(Lines 9-14 in that file) derives it fromoutputOptions.format === 'es'(with fileName only as a secondary trigger to enter the branch). If a build config'sformatand emitted file extension ever diverge (e.g., customentryFileNames/chunkFileNamespatterns, or non-es/non-cjsformats likeiife), these two plugins operating on the same chunk could disagree on whether to emitimport/fromvsrequire(...)for core imports, producing broken output.Consider centralizing ESM detection into a single shared helper both plugins call, to guarantee consistent behavior.
🤖 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 `@config/plugins/rolldown/resolveCoreImportsPlugin.ts` at line 10, The ESM/CJS decision in resolveCoreImportsPlugin is inconsistent with forceFileExtensionsPlugin because isEsm is derived only from chunk.fileName.endsWith(ESM_EXTENSION) here. Update resolveCoreImportsPlugin to use the same shared ESM detection logic as forceFileExtensionsPlugin, ideally via a common helper that checks outputOptions.format first and only falls back to the file-name extension when needed. Keep the shared helper referenced from both plugins so core-import emission stays consistent for the same chunk regardless of naming patterns or format.
🤖 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.
Inline comments:
In `@config/plugins/rolldown/forceFileExtensionsPlugin.ts`:
- Around line 9-19: The ESM check in renderChunk uses two different signals,
which can cause imports to be rewritten with the wrong module assumption.
Compute a single effective ESM flag in forceFileExtensionsPlugin using both
chunk.fileName and outputOptions.format, then use that same value for both the
early return condition and the modifyRelativeImports call so the branch decision
and transform input stay consistent.
In `@config/plugins/rolldown/graphQLImportPlugin.ts`:
- Around line 12-24: The early-return check in graphQLImportPlugin.transform
only detects require('graphql'), so files using require("graphql") are skipped
even though the replacement logic already supports both quote styles. Update the
guard to match the same cases as the replace regex, or otherwise detect any
graphql require in the source before returning, so the transform runs
consistently for both single- and double-quoted imports.
In `@tsdown.config.mts`:
- Around line 116-145: The browserConfig.deps.alwaysBundle predicate is checking
package names with mswCore.test, which won’t filter the intended dependencies
and can bundle everything. Update the logic in browserConfig so alwaysBundle
uses the package-name matcher that aligns with ecosystemDependencies instead of
mswCore.test, while keeping neverBundle and onlyBundle behavior unchanged. Use
the browserConfig.deps block and the ecosystemDependencies symbol to locate the
fix.
---
Nitpick comments:
In `@config/plugins/rolldown/resolveCoreImportsPlugin.ts`:
- Line 10: The ESM/CJS decision in resolveCoreImportsPlugin is inconsistent with
forceFileExtensionsPlugin because isEsm is derived only from
chunk.fileName.endsWith(ESM_EXTENSION) here. Update resolveCoreImportsPlugin to
use the same shared ESM detection logic as forceFileExtensionsPlugin, ideally
via a common helper that checks outputOptions.format first and only falls back
to the file-name extension when needed. Keep the shared helper referenced from
both plugins so core-import emission stays consistent for the same chunk
regardless of naming patterns or format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1c6a8d98-b712-4824-8f56-02e968b938d1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
config/plugins/esbuild/graphQLImportPlugin.tsconfig/plugins/esbuild/resolveCoreImportsPlugin.tsconfig/plugins/rolldown/copyWorkerPlugin.tsconfig/plugins/rolldown/forceFileExtensionsPlugin.tsconfig/plugins/rolldown/graphQLImportPlugin.tsconfig/plugins/rolldown/resolveCoreImportsPlugin.tspackage.jsontsdown.config.mtstsup.config.ts
💤 Files with no reviewable changes (3)
- config/plugins/esbuild/resolveCoreImportsPlugin.ts
- tsup.config.ts
- config/plugins/esbuild/graphQLImportPlugin.ts
| renderChunk(code, chunk, outputOptions) { | ||
| const isEsm = outputOptions.format === 'es' | ||
|
|
||
| build.onEnd(async (result) => { | ||
| if (result.errors.length > 0) { | ||
| return | ||
| } | ||
|
|
||
| for (const outputFile of result.outputFiles || []) { | ||
| // Only target CJS/ESM files. | ||
| // This ignores additional files emitted, like sourcemaps ("*.js.map"). | ||
| if ( | ||
| !( | ||
| outputFile.path.endsWith(ESM_EXTENSION) || | ||
| outputFile.path.endsWith('.mjs') | ||
| ) | ||
| ) { | ||
| continue | ||
| } | ||
|
|
||
| const fileContents = outputFile.text | ||
| const nextFileContents = modifyRelativeImports(fileContents, isEsm) | ||
| if (!(chunk.fileName.endsWith(ESM_EXTENSION) || isEsm)) { | ||
| return | ||
| } | ||
|
|
||
| outputFile.contents = Buffer.from(nextFileContents) | ||
| } | ||
| }) | ||
| return { | ||
| code: modifyRelativeImports(code, isEsm), | ||
| map: null, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Entry condition and transform value use different ESM signals.
Line 12 enters the branch if either chunk.fileName.endsWith(ESM_EXTENSION) or isEsm (format-based) is true, but Line 17 only passes the format-based isEsm into modifyRelativeImports. If a chunk's file name ends in .mjs while outputOptions.format isn't 'es' (or vice versa, an edge case with custom entryFileNames), the function processes the chunk but rewrites imports using the wrong assumption of module format.
♻️ Proposed fix — compute effective ESM state once
renderChunk(code, chunk, outputOptions) {
- const isEsm = outputOptions.format === 'es'
+ const isEsm =
+ outputOptions.format === 'es' || chunk.fileName.endsWith(ESM_EXTENSION)
- if (!(chunk.fileName.endsWith(ESM_EXTENSION) || isEsm)) {
+ if (!isEsm) {
return
}📝 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.
| renderChunk(code, chunk, outputOptions) { | |
| const isEsm = outputOptions.format === 'es' | |
| build.onEnd(async (result) => { | |
| if (result.errors.length > 0) { | |
| return | |
| } | |
| for (const outputFile of result.outputFiles || []) { | |
| // Only target CJS/ESM files. | |
| // This ignores additional files emitted, like sourcemaps ("*.js.map"). | |
| if ( | |
| !( | |
| outputFile.path.endsWith(ESM_EXTENSION) || | |
| outputFile.path.endsWith('.mjs') | |
| ) | |
| ) { | |
| continue | |
| } | |
| const fileContents = outputFile.text | |
| const nextFileContents = modifyRelativeImports(fileContents, isEsm) | |
| if (!(chunk.fileName.endsWith(ESM_EXTENSION) || isEsm)) { | |
| return | |
| } | |
| outputFile.contents = Buffer.from(nextFileContents) | |
| } | |
| }) | |
| return { | |
| code: modifyRelativeImports(code, isEsm), | |
| map: null, | |
| } | |
| renderChunk(code, chunk, outputOptions) { | |
| const isEsm = | |
| outputOptions.format === 'es' || chunk.fileName.endsWith(ESM_EXTENSION) | |
| if (!isEsm) { | |
| return | |
| } | |
| return { | |
| code: modifyRelativeImports(code, isEsm), | |
| map: null, | |
| } |
🤖 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 `@config/plugins/rolldown/forceFileExtensionsPlugin.ts` around lines 9 - 19,
The ESM check in renderChunk uses two different signals, which can cause imports
to be rewritten with the wrong module assumption. Compute a single effective ESM
flag in forceFileExtensionsPlugin using both chunk.fileName and
outputOptions.format, then use that same value for both the early return
condition and the modifyRelativeImports call so the branch decision and
transform input stay consistent.
| const browserConfig: UserConfig = { | ||
| ...commonConfig, | ||
| name: 'browser', | ||
| platform: 'browser', | ||
| entry: ['./src/browser/index.ts'], | ||
| deps: { | ||
| neverBundle: [mswCore, ecosystemDependencies], | ||
| alwaysBundle: Object.keys(packageJson.dependencies).filter((packageName) => { | ||
| return !mswCore.test(packageName) | ||
| }), | ||
| onlyBundle: false, | ||
| }, | ||
| format: ['esm', 'cjs'], | ||
| outDir: './lib/browser', | ||
| unbundle: false, | ||
| outputOptions: { | ||
| codeSplitting: false, | ||
| }, | ||
| sourcemap: true, | ||
| dts: { build: true }, | ||
| tsconfig: path.resolve(__dirname, 'src/browser/tsconfig.browser.build.json'), | ||
| define: { | ||
| SERVICE_WORKER_CHECKSUM: JSON.stringify(SERVICE_WORKER_CHECKSUM), | ||
| }, | ||
| plugins: [ | ||
| resolveCoreImportsPlugin(), | ||
| forceFileExtensionsPlugin(), | ||
| copyWorkerPlugin(SERVICE_WORKER_CHECKSUM), | ||
| ], | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
browserConfig.deps.alwaysBundle uses the wrong predicate
Object.keys(packageJson.dependencies) yields bare package names, so mswCore.test(packageName) will never exclude anything here. That forces every dependency into alwaysBundle, including packages that neverBundle is meant to keep external. ecosystemDependencies is the matching predicate for package names.
🐛 Proposed fix
alwaysBundle: Object.keys(packageJson.dependencies).filter((packageName) => {
- return !mswCore.test(packageName)
+ return !ecosystemDependencies.test(packageName)
}),📝 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.
| const browserConfig: UserConfig = { | |
| ...commonConfig, | |
| name: 'browser', | |
| platform: 'browser', | |
| entry: ['./src/browser/index.ts'], | |
| deps: { | |
| neverBundle: [mswCore, ecosystemDependencies], | |
| alwaysBundle: Object.keys(packageJson.dependencies).filter((packageName) => { | |
| return !mswCore.test(packageName) | |
| }), | |
| onlyBundle: false, | |
| }, | |
| format: ['esm', 'cjs'], | |
| outDir: './lib/browser', | |
| unbundle: false, | |
| outputOptions: { | |
| codeSplitting: false, | |
| }, | |
| sourcemap: true, | |
| dts: { build: true }, | |
| tsconfig: path.resolve(__dirname, 'src/browser/tsconfig.browser.build.json'), | |
| define: { | |
| SERVICE_WORKER_CHECKSUM: JSON.stringify(SERVICE_WORKER_CHECKSUM), | |
| }, | |
| plugins: [ | |
| resolveCoreImportsPlugin(), | |
| forceFileExtensionsPlugin(), | |
| copyWorkerPlugin(SERVICE_WORKER_CHECKSUM), | |
| ], | |
| } | |
| const browserConfig: UserConfig = { | |
| ...commonConfig, | |
| name: 'browser', | |
| platform: 'browser', | |
| entry: ['./src/browser/index.ts'], | |
| deps: { | |
| neverBundle: [mswCore, ecosystemDependencies], | |
| alwaysBundle: Object.keys(packageJson.dependencies).filter((packageName) => { | |
| return !ecosystemDependencies.test(packageName) | |
| }), | |
| onlyBundle: false, | |
| }, | |
| format: ['esm', 'cjs'], | |
| outDir: './lib/browser', | |
| unbundle: false, | |
| outputOptions: { | |
| codeSplitting: false, | |
| }, | |
| sourcemap: true, | |
| dts: { build: true }, | |
| tsconfig: path.resolve(__dirname, 'src/browser/tsconfig.browser.build.json'), | |
| define: { | |
| SERVICE_WORKER_CHECKSUM: JSON.stringify(SERVICE_WORKER_CHECKSUM), | |
| }, | |
| plugins: [ | |
| resolveCoreImportsPlugin(), | |
| forceFileExtensionsPlugin(), | |
| copyWorkerPlugin(SERVICE_WORKER_CHECKSUM), | |
| ], | |
| } |
🤖 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 `@tsdown.config.mts` around lines 116 - 145, The
browserConfig.deps.alwaysBundle predicate is checking package names with
mswCore.test, which won’t filter the intended dependencies and can bundle
everything. Update the logic in browserConfig so alwaysBundle uses the
package-name matcher that aligns with ecosystemDependencies instead of
mswCore.test, while keeping neverBundle and onlyBundle behavior unchanged. Use
the browserConfig.deps block and the ecosystemDependencies symbol to locate the
fix.
There was a problem hiding this comment.
Strange but both current state and suggested fix result in the same lib/browser/index.mjs content. Is it because neverBundle takes priority over alwaysBundle, @Andarist? If that's the case, do we need filtering for packageJson.dependencies at all? Should it be just
neverBundle: [mswCore, ecosystemDependencies],
alwaysBundle: Object.keys(packageJson.dependencies),There was a problem hiding this comment.
🧩 Analysis chain
🤖 web_search executed:
tsdown deps.neverBundle alwaysBundle precedence which takes priority
Length of output: 10092
🏁 Script executed:
#!/bin/bash
# Try to find tsdown package source for deps resolution logic if installed or check GitHub repo
fd -HI 'tsdown' -t d 2>/dev/null | head -20
rg -n "neverBundle|alwaysBundle|onlyBundle" --type=ts -C2 2>/dev/null | head -100Length of output: 482
🤖 web_search executed:
tsdown source code where resolved deps neverBundle and alwaysBundle combined into rolldown external option resolveId
Length of output: 19279
🤖 web_search executed:
tsdown github src/features/external.ts neverBundle resolveId external plugin latest source full file
Length of output: 7660
@kettanaito, you're right — neverBundle does take priority here, and it's not really about alwaysBundle's ordering.
Looking at tsdown's source (src/features/external.ts), the deps.alwaysBundle (mapped internally to noExternal) is only checked inside tsdown:external's own resolveId plugin hook, and even there, a match just returns 'no-external' (a signal used for inlineOnly bookkeeping) — it doesn't forcibly un-externalize anything.
deps.neverBundle (the deprecated external option), on the other hand, is passed straight through to Rolldown's native external resolution config, which Rolldown applies during its own module resolution before any user plugin's resolveId hook — including tsdown:external — gets a chance to run. Once Rolldown's core resolver has already marked an id external via neverBundle, the alwaysBundle plugin logic never gets an opportunity to override it.
So yes — since neverBundle: [mswCore, ecosystemDependencies] already guarantees those patterns stay external, filtering alwaysBundle (by either predicate) is redundant. Your simplification is the better version:
deps: {
neverBundle: [mswCore, ecosystemDependencies],
alwaysBundle: Object.keys(packageJson.dependencies),
onlyBundle: false,
},This removes the confusing/incorrect filter entirely and relies on neverBundle to do the actual exclusion, which is both simpler and correctly reflects how these options interact. I'd go with this.
I'll retract the "Major" severity on my original suggestion — the predicate was indeed wrong, but it had no functional impact given neverBundle's precedence, so there was no actual bundling bug in practice.
✏️ Learnings added
Learnt from: kettanaito
Repo: mswjs/msw PR: 2760
File: tsdown.config.mts:116-145
Timestamp: 2026-07-06T12:47:13.540Z
Learning: In `tsdown.config.mts` (and generally when configuring tsdown's `deps` option), `deps.neverBundle` takes precedence over `deps.alwaysBundle` for entries matching both. This is because `neverBundle` maps to Rolldown's native `external` build option, resolved by Rolldown's core resolver before any user plugin's `resolveId` hook (including tsdown's internal `alwaysBundle`/`noExternal` handling) runs. As a result, filtering `alwaysBundle` to exclude packages already covered by `neverBundle` regexes (e.g. via `ecosystemDependencies.test` or `mswCore.test`) is redundant — those packages stay external regardless of what's listed in `alwaysBundle`. Prefer simply listing all `packageJson.dependencies` in `alwaysBundle` without filtering, and let `neverBundle` handle exclusions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
I pushed the change anyway because matching against mswCore is clearly a bug. The pending question is whether this filter is even needed (see above).
kettanaito
changed the title
2 participants
I looked through dist files’ diffs and, from what i can tell, the output looks pretty much the same - minis some stylistic~ differences.
tsup seems to be somewhat unmaintained these days and has some bugs that blocked me from doing build-related refactors. So my hope is that the switch to tsdown will unblock that work