feat(bun)!: sanitize plugin names for Bun namespaces#599
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA local helper ChangesBun namespace sanitization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/bun/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/bun/index.ts`:
- Around line 225-229: The loop registering build.onLoad for
virtualModulePlugins can produce duplicate namespaces via
toBunNamespace(pluginName) which causes collisions; before registering, compute
the namespace for each pluginName (using toBunNamespace) and detect duplicates
(e.g., build a Set or map keyed by namespace), and if a duplicate is found
either throw a clear error mentioning the conflicting plugin names or generate a
deterministic unique namespace (e.g., append a stable suffix) and use that when
calling build.onLoad; update any related uses of processLoadTransform to receive
the chosen namespace mapping so the correct pluginName is passed to
processLoadTransform.
- Around line 9-13: The toBunNamespace function can return an empty string when
plugin.name is empty, which violates Bun's non-empty namespace requirement;
update toBunNamespace to check if the input (and the sanitized result) is empty
and return a safe non-empty fallback (e.g., "-" or "unnamed-plugin") when that
happens, ensuring any callsites that pass plugin.name (reference plugin.name)
receive a validator-compliant string matching Bun's /^([/$a-zA-Z0-9_\\-]+)$/
rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| for (const pluginName of virtualModulePlugins) { | ||
| build.onLoad({ filter: /.*/, namespace: pluginName }, async (args) => { | ||
| build.onLoad({ filter: /.*/, namespace: toBunNamespace(pluginName) }, async (args) => { | ||
| return processLoadTransform(args.path, pluginName, args.loader) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential namespace collision risk with multiple plugins.
If two plugins have names that sanitize to the same namespace (e.g., "foo@bar" and "foo-bar" both become "foo-bar"), the second build.onLoad registration could conflict with the first. This may result in one plugin's load hook shadowing another or runtime errors.
Consider adding collision detection:
🛡️ Proposed collision detection
+ const sanitizedNamespaces = new Map<string, string>()
for (const pluginName of virtualModulePlugins) {
+ const sanitized = toBunNamespace(pluginName)
+ const existing = sanitizedNamespaces.get(sanitized)
+ if (existing) {
+ console.warn(
+ `[unplugin] Namespace collision detected: plugins "${existing}" and "${pluginName}" both sanitize to "${sanitized}". This may cause unexpected behavior.`
+ )
+ }
+ sanitizedNamespaces.set(sanitized, pluginName)
build.onLoad({ filter: /.*/, namespace: toBunNamespace(pluginName) }, async (args) => {
return processLoadTransform(args.path, pluginName, args.loader)
})
}📝 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.
| for (const pluginName of virtualModulePlugins) { | |
| build.onLoad({ filter: /.*/, namespace: pluginName }, async (args) => { | |
| build.onLoad({ filter: /.*/, namespace: toBunNamespace(pluginName) }, async (args) => { | |
| return processLoadTransform(args.path, pluginName, args.loader) | |
| }) | |
| } | |
| const sanitizedNamespaces = new Map<string, string>() | |
| for (const pluginName of virtualModulePlugins) { | |
| const sanitized = toBunNamespace(pluginName) | |
| const existing = sanitizedNamespaces.get(sanitized) | |
| if (existing) { | |
| console.warn( | |
| `[unplugin] Namespace collision detected: plugins "${existing}" and "${pluginName}" both sanitize to "${sanitized}". This may cause unexpected behavior.` | |
| ) | |
| } | |
| sanitizedNamespaces.set(sanitized, pluginName) | |
| build.onLoad({ filter: /.*/, namespace: toBunNamespace(pluginName) }, async (args) => { | |
| return processLoadTransform(args.path, pluginName, args.loader) | |
| }) | |
| } |
🤖 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 `@src/bun/index.ts` around lines 225 - 229, The loop registering build.onLoad
for virtualModulePlugins can produce duplicate namespaces via
toBunNamespace(pluginName) which causes collisions; before registering, compute
the namespace for each pluginName (using toBunNamespace) and detect duplicates
(e.g., build a Set or map keyed by namespace), and if a duplicate is found
either throw a clear error mentioning the conflicting plugin names or generate a
deterministic unique namespace (e.g., append a stable suffix) and use that when
calling build.onLoad; update any related uses of processLoadTransform to receive
the chosen namespace mapping so the correct pluginName is passed to
processLoadTransform.
There was a problem hiding this comment.
yes, this is an intentional side effect of doing this
There was a problem hiding this comment.
🧩 Analysis chain
@81reap, understood — thanks for clarifying! I'll go ahead and resolve this comment.
🏁 Script executed:
#!/bin/bash
# List CodeRabbit's comments on PR `#599` to find the one to resolve
gh pr view 599 --repo unjs/unplugin --comments --json comments --jq '.comments[] | select(.author.login == "coderabbitai[bot]") | {id: .id, url: .url, body: .body[:100]}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# List review comments (inline) on PR `#599`
gh api repos/unjs/unplugin/pulls/599/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, url: .html_url, body: .body[:80]}'
Length of output: 390
🏁 Script executed:
#!/bin/bash
# Get the pull request review thread node ID for the comment
gh api graphql -f query='
{
repository(owner: "unjs", name: "unplugin") {
pullRequest(number: 599) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].databaseId == 3285099044) | {threadId: .id, isResolved: .isResolved}'
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Resolve the review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOFvaecc6D9-0h"}) {
thread {
id
isResolved
}
}
}'
Length of output: 266
@81reap, understood — thanks for clarifying! I wasn't able to resolve the thread programmatically due to permission restrictions, but I'll note this as intentional behavior.
The trade-off is clear: sanitizing plugin names to meet Bun's namespace requirements is the primary goal, and any potential collisions from two distinctly-named plugins mapping to the same sanitized namespace are an accepted edge case. If collisions ever become a real problem in practice, a warning or deduplication strategy can be revisited in a follow-up.
🧬 Code Graph Analysis Results
- File: test/fixtures/load/bun.config.js (lines 1-1)
const Bun = require('bun')
- File: test/fixtures/transform/bun.config.js (lines 1-1)
const Bun = require('bun')
- File: test/fixtures/virtual-module/bun.config.js (lines 1-1)
const Bun = require('bun')
- File: src/bun/utils.ts (lines 29-55)
export function createBuildContext(build: PluginBuilder): UnpluginBuildContext {
const watchFiles: string[] = []
return {
addWatchFile(file) {
watchFiles.push(file)
},
getWatchFiles() {
return watchFiles
},
emitFile(emittedFile) {
const outFileName = emittedFile.fileName || emittedFile.name
const outdir = build?.config?.outdir
if (outdir && emittedFile.source && outFileName) {
const outPath = path.resolve(outdir, outFileName)
const outDir = path.dirname(outPath)
if (!fs.existsSync(outDir))
fs.mkdirSync(outDir, { recursive: true })
fs.writeFileSync(outPath, emittedFile.source)
}
},
parse,
getNativeBuildContext() {
return { framework: 'bun', build }
},
}
}
- File: src/bun/utils.ts (lines 57-82)
export function createPluginContext(
buildContext: UnpluginBuildContext,
): {
errors: Array<string | UnpluginMessage>
warnings: Array<string | UnpluginMessage>
mixedContext: UnpluginBuildContext & UnpluginContext
} {
const errors: Array<string | UnpluginMessage> = []
const warnings: Array<string | UnpluginMessage> = []
const mixedContext: UnpluginBuildContext & UnpluginContext = {
...buildContext,
error(error) {
errors.push(error)
},
warn(warning) {
warnings.push(warning)
},
}
return {
errors,
warnings,
mixedContext,
}
}
- File: src/utils/general.ts (lines 3-8)
export function toArray<T>(array?: Nullable<Arrayable<T>>): Array<T> {
array = array || []
if (Array.isArray(array))
return array
return [array]
}
- File: src/utils/filter.ts (lines 153-181)
export function normalizeObjectHook<T extends (...args: any[]) => any, F extends keyof HookFilter>(
name: 'resolveId' | 'load' | 'transform',
hook: Hook<T, F>,
): {
handler: T
filter: PluginFilter | TransformHookFilter
} {
let handler: T
let filter: PluginFilter | TransformHookFilter | undefined
if (typeof hook === 'function') {
handler = hook
}
else {
handler = hook.handler
const hookFilter = hook.filter as HookFilter | undefined
if (name === 'resolveId' || name === 'load') {
filter = createFilterForId(hookFilter?.id)
}
else {
filter = createFilterForTransform(hookFilter?.id, hookFilter?.code)
}
}
return {
handler,
filter: filter || (() => true),
}
}
- File: test/unit-tests/utils.ts (lines 24-49)
export const build: {
webpack: typeof webpack.webpack
rspack: typeof rspackBuild
rollup: typeof rollupBuild
rolldown: typeof rolldownBuild
vite: typeof viteBuild
esbuild: typeof esbuildBuild
bun: typeof bunBuild
} = {
webpack: webpackBuild,
rspack: rspackBuild,
rollup: rollupBuild,
rolldown: rolldownBuild,
vite(config) {
return viteBuild(vite.mergeConfig(config || {}, {
build: {
rollupOptions: {
logLevel: 'silent',
},
},
logLevel: 'silent',
}))
},
esbuild: esbuildBuild,
bun: bunBuild,
}
- File: src/types.ts (lines 158-161)
export type UnpluginFactory<UserOptions, Nested extends boolean = boolean> = (options: UserOptions, meta: UnpluginContextMeta) =>
Nested extends true
? Array<UnpluginOptions>
: UnpluginOptions
- File: src/types.ts (lines 166-177)
export interface UnpluginInstance<UserOptions, Nested extends boolean = boolean> {
rollup: UnpluginFactoryOutput<UserOptions, Nested extends true ? Array<RollupPlugin> : RollupPlugin>
vite: UnpluginFactoryOutput<UserOptions, Nested extends true ? Array<VitePlugin> : VitePlugin>
rolldown: UnpluginFactoryOutput<UserOptions, Nested extends true ? Array<RolldownPlugin> : RolldownPlugin>
webpack: UnpluginFactoryOutput<UserOptions, WebpackPluginInstance>
rspack: UnpluginFactoryOutput<UserOptions, RspackPluginInstance>
esbuild: UnpluginFactoryOutput<UserOptions, EsbuildPlugin>
unloader: UnpluginFactoryOutput<UserOptions, Nested extends true ? Array<UnloaderPlugin> : UnloaderPlugin>
farm: UnpluginFactoryOutput<UserOptions, FarmPlugin>
bun: UnpluginFactoryOutput<UserOptions, BunPlugin>
raw: UnpluginFactory<UserOptions, Nested>
}
- File: src/types.ts (lines 181-209)
export type UnpluginContextMeta = Partial<RollupContextMeta> & {
/**
* Version information for frameworks.
* Access the current framework version via: `meta.versions[meta.framework]`
*
* For Vite, includes both Vite's version and the underlying bundler (Rollup/Rolldown).
* For Rollup-compatible frameworks (vite, rollup, rolldown, unloader),
* versions are only available after the `buildStart` hook.
*
* The `unplugin` version is always available for all frameworks.
*/
versions: Partial<Record<SupportedFramework | 'unplugin', string>>
} & ({
framework: 'rollup' | 'vite' | 'rolldown' | 'farm' | 'unloader'
} | {
framework: 'webpack'
webpack: { compiler: WebpackCompiler }
} | {
framework: 'esbuild'
/** Set the host plugin name of esbuild when returning multiple plugins */
esbuildHostName?: string | undefined
} | {
framework: 'bun'
/** Set the host plugin name of bun when returning multiple plugins */
bunHostName?: string | undefined
} | {
framework: 'rspack'
rspack: { compiler: RspackCompiler }
})
- File: src/types.ts (lines 48-48)
export type TransformResult = string | { code: string, map?: SourceMapInput | SourceMapCompact | null | undefined } | null | undefined | void
✏️ Learnings added
Learnt from: 81reap
Repo: unjs/unplugin PR: 599
File: src/bun/index.ts:225-229
Timestamp: 2026-05-22T00:41:53.946Z
Learning: In `src/bun/index.ts` of the unjs/unplugin repository, the `toBunNamespace()` helper replaces invalid characters in plugin names to produce Bun-compatible namespace identifiers. The potential for two different plugin names sanitizing to the same namespace (e.g., "foobar" and "foo-bar" both becoming "foo-bar") is an intentional, accepted side effect of this sanitization approach — not a bug to be fixed.
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.
also @sxzz I would like to talk more about this fix before merging (the other two are straight forward). there are intentional side effects being added to the codebase. Not sure if your aligned with it or have recommended safety precautions to keep a high bar for good UX/DX
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think this is acceptable in this case. Previously, namespaces containing @ would directly cause Bun to throw an error, so this change will not make the situation worse.
commit: |
|
Please fix lint errors and rename the PR title to respect conventional commit message. |
|
Will do, just to clarify, what is the standard commit message? 
|
|
See https://www.conventionalcommits.org/en/v1.0.0/ P.S: No, it's this PR #601 |
|
thank you for sharing! I'll work on getting that fixed
sorry, I'm not sure I follow. Are you asking me to also update that PR commit message? afaik they should be separate issues/bugs |
|
Yes, I realized I misread it. This PR's title seems fine, I was referring to the other PR. |
Bun only allows `\w`, `$`, and `-` in plugin namespaces, so coerce unsupported characters in `plugin.name` to `-` before using it as a namespace. BREAKING CHANGE: Sanitized plugin names may have namespace collisions if existing plugins map to the same name.
81reap
force-pushed
the
feat/bun-adapter-improvements
branch
from
1ceaadd to
db86f6e
Compare
|
No worries, I pushed up a new revision that fixes the lint issues, adds unit tests, and updates the commit message to call out the potential breaking change. let me take a stab at the other one before I go into work |
81reap
changed the title
2 participants
I am volunteering and working on getting a bun plugin for TanStack/router and found this bug.
plugin.namewith characters outside[$a-zA-Z0-9_-]crashes onLoad #598Summary by CodeRabbit