-
Notifications
You must be signed in to change notification settings - Fork 148
feat(bun)!: sanitize plugin names for Bun namespaces #599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,12 @@ import { normalizeObjectHook } from '../utils/filter' | |||||||||||||||||||||||||||||||||||||||||
| import { toArray } from '../utils/general' | ||||||||||||||||||||||||||||||||||||||||||
| import { createBuildContext, createPluginContext, guessLoader } from './utils' | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Coerce plugin.name to satisfy Bun's namespace validator: | ||||||||||||||||||||||||||||||||||||||||||
| // https://github.com/oven-sh/bun/blob/12d77d1ac561771e9fa1d0822e954273248e7f9a/src/js/builtins/BundlerPlugin.ts#L215-L217 | ||||||||||||||||||||||||||||||||||||||||||
| function toBunNamespace(name: string): string { | ||||||||||||||||||||||||||||||||||||||||||
| return name.replace(/[^\w$-]/g, '-') | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| export function getBunPlugin<UserOptions = Record<string, never>>( | ||||||||||||||||||||||||||||||||||||||||||
| factory: UnpluginFactory<UserOptions>, | ||||||||||||||||||||||||||||||||||||||||||
| ): UnpluginInstance<UserOptions>['bun'] { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -106,7 +112,7 @@ export function getBunPlugin<UserOptions = Record<string, never>>( | |||||||||||||||||||||||||||||||||||||||||
| if (!isAbsolute(result)) { | ||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||
| path: result, | ||||||||||||||||||||||||||||||||||||||||||
| namespace: plugin.name, | ||||||||||||||||||||||||||||||||||||||||||
| namespace: toBunNamespace(plugin.name), | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return { path: result } | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -116,7 +122,7 @@ export function getBunPlugin<UserOptions = Record<string, never>>( | |||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||
| path: result.id, | ||||||||||||||||||||||||||||||||||||||||||
| external: result.external, | ||||||||||||||||||||||||||||||||||||||||||
| namespace: plugin.name, | ||||||||||||||||||||||||||||||||||||||||||
| namespace: toBunNamespace(plugin.name), | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -221,7 +227,7 @@ export function getBunPlugin<UserOptions = Record<string, never>>( | |||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
229
to
233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential namespace collision risk with multiple plugins. If two plugins have names that sanitize to the same namespace (e.g., 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
Suggested change
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this is an intentional side effect of doing this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain
🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 390 🏁 Script executed: Length of output: 56 🏁 Script executed: Length of output: 266
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
✏️ Learnings added
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is acceptable in this case. Previously, namespaces containing |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| import { createUnplugin } from 'unplugin' | ||
| import { describe, expect, it, vi } from 'vitest' | ||
|
|
||
| interface MockBuild { | ||
| build: Bun.PluginBuilder | ||
| resolveCallback: () => Bun.OnResolveCallback | ||
| loadCallbacks: Map<string, Bun.OnLoadCallback> | ||
| } | ||
|
|
||
| function createMockBuild(): MockBuild { | ||
| let resolveCallback: Bun.OnResolveCallback | undefined | ||
| const loadCallbacks = new Map<string, Bun.OnLoadCallback>() | ||
|
|
||
| const build = { | ||
| onResolve: vi.fn((_options, callback) => { | ||
| resolveCallback = callback | ||
| }), | ||
| onLoad: vi.fn((options: { namespace?: string }, callback: Bun.OnLoadCallback) => { | ||
| if (options.namespace) { | ||
| loadCallbacks.set(options.namespace, callback) | ||
| } | ||
| }), | ||
| onStart: vi.fn(), | ||
| config: { outdir: './dist' }, | ||
| } as never as Bun.PluginBuilder | ||
|
|
||
| return { | ||
| build, | ||
| resolveCallback: () => { | ||
| if (!resolveCallback) { | ||
| throw new Error('onResolve was not registered') | ||
| } | ||
| return resolveCallback | ||
| }, | ||
| loadCallbacks, | ||
| } | ||
| } | ||
|
|
||
| describe.skipIf(typeof Bun === 'undefined')('bun namespace sanitization', () => { | ||
| it('should sanitize invalid characters when resolveId returns a string', async () => { | ||
| const unplugin = createUnplugin(() => ({ | ||
| name: 'unplugin:my.plugin/name', | ||
| resolveId: () => 'virtual-id', | ||
| })) | ||
| const { build, resolveCallback } = createMockBuild() | ||
|
|
||
| await unplugin.bun().setup(build) | ||
| const result = await resolveCallback()({ | ||
| path: 'foo', | ||
| importer: 'index.js', | ||
| kind: 'import-statement', | ||
| } as Bun.OnResolveArgs) | ||
|
|
||
| expect(result).toEqual({ | ||
| path: 'virtual-id', | ||
| namespace: 'unplugin-my-plugin-name', | ||
| }) | ||
| }) | ||
|
|
||
| it('should sanitize invalid characters when resolveId returns an object', async () => { | ||
| const unplugin = createUnplugin(() => ({ | ||
| name: '@scope/plugin.name', | ||
| resolveId: () => ({ id: 'virtual-id', external: false }), | ||
| })) | ||
| const { build, resolveCallback } = createMockBuild() | ||
|
|
||
| await unplugin.bun().setup(build) | ||
| const result = await resolveCallback()({ | ||
| path: 'foo', | ||
| importer: 'index.js', | ||
| kind: 'import-statement', | ||
| } as Bun.OnResolveArgs) | ||
|
|
||
| expect(result).toEqual({ | ||
| path: 'virtual-id', | ||
| external: false, | ||
| namespace: '-scope-plugin-name', | ||
| }) | ||
| }) | ||
|
|
||
| it('should leave plugin names with only allowed characters untouched', async () => { | ||
| const unplugin = createUnplugin(() => ({ | ||
| name: 'valid_plugin-name$1', | ||
| resolveId: () => 'virtual-id', | ||
| })) | ||
| const { build, resolveCallback } = createMockBuild() | ||
|
|
||
| await unplugin.bun().setup(build) | ||
| const result = await resolveCallback()({ | ||
| path: 'foo', | ||
| importer: 'index.js', | ||
| kind: 'import-statement', | ||
| } as Bun.OnResolveArgs) | ||
|
|
||
| expect(result).toEqual({ | ||
| path: 'virtual-id', | ||
| namespace: 'valid_plugin-name$1', | ||
| }) | ||
| }) | ||
|
|
||
| it('should invoke the original load hook when registered under a sanitized namespace', async () => { | ||
| const load = vi.fn(() => 'export default 1') | ||
| const unplugin = createUnplugin(() => ({ | ||
| name: 'unplugin:virtual.mod', | ||
| resolveId: () => 'virtual-id', | ||
| load, | ||
| })) | ||
| const { build, loadCallbacks } = createMockBuild() | ||
|
|
||
| await unplugin.bun().setup(build) | ||
|
|
||
| expect([...loadCallbacks.keys()]).toContain('unplugin-virtual-mod') | ||
| expect([...loadCallbacks.keys()]).not.toContain('unplugin:virtual.mod') | ||
|
|
||
| const result = await loadCallbacks.get('unplugin-virtual-mod')!({ | ||
| path: 'virtual-id', | ||
| loader: 'js', | ||
| } as Bun.OnLoadArgs) | ||
|
|
||
| expect(load).toHaveBeenCalledWith('virtual-id') | ||
| expect(result).toEqual({ | ||
| contents: 'export default 1', | ||
| loader: 'js', | ||
| }) | ||
| }) | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.