Repository navigation
Conversation
`JSBundlerPlugin__create` took a `BunPluginTarget` and stored it in `BundlerPlugin::target`. No code reads that member. The `Bun.plugin` helpers in BunPlugin.cpp took the same type as a parameter and did not use it. Drop the parameter from `JSBundlerPlugin__create`, `JSBundlerPlugin::create`, both constructors, `PluginJscExt::create` and the `Bun.plugin` helpers. Delete the four `target` members, the Rust `BunPluginTarget` enum and the C++ `BunPluginTarget` typedef with its constants. Delete the host functions `jsFunctionAppendOnLoadPluginNode`, `jsFunctionAppendOnLoadPluginBrowser`, `jsFunctionAppendOnResolvePluginNode` and `jsFunctionAppendOnResolvePluginBrowser`. They have no caller, and without the parameter they are copies of the `Bun` pair. `Bun.plugin` still validates the `target` string, and `Bun.build` still reads its own `target` option.
|
Updated 6:56 AM PT - Oct 21st, 2026
✅ @robobun, your commit 2706694f0ff780200e342bab0be132f433b1004f passed in 🧪 To try this PR locally: bunx bun-pr 46089That installs a local version of the PR into your bun-46089 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — mechanical removal of a value nothing reads; the Rust extern "C" declaration and the C++ JSBundlerPlugin__create definition still agree parameter-for-parameter, and no reference to BunPluginTarget or the four deleted host functions remains in the tree.
Extended reasoning...
The change deletes the unused BunPluginTarget type, four unread target members, the matching parameters on JSBundlerPlugin::create/constructors/JSBundlerPlugin__create and the Bun.plugin helper chain, and four uncalled host functions, then updates the three Rust call sites. It touches no security-sensitive surface: no user input handling changes, and the Bun.plugin({ target }) validation and error message are untouched. I confirmed the FFI signature on both sides matches, that bun_ast::Target remains used in JSBundler.rs so the import is live, and that no changed path is covered by CODEOWNERS. The one inline finding concerns a pre-existing SAFETY comment on the line below the edited call in server_body.rs, which this PR did not introduce, so it does not change the correctness of the removal itself.
|
Status: ready for review.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughPlugin creation no longer passes a target through the plugin API. Bun registration uses Bun onLoad and onResolve handlers. Bundler and server call sites use the updated target-independent creation function. ChangesPlugin creation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the primary change: removal of the unused BunPluginTarget value. |
| Description check | |
The description clearly explains the problem, fix, retained behavior, verification steps, test results, and known limitations. It does not use the exact template headings, but it provides the required… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description clearly explains the problem, fix, retained behavior, verification steps, test results, and known limitations. It does not use the exact template headings, but it provides the required information in equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
The comment said that `Plugin::create` returns a boxed pointer. It returns a protected JSC cell. `heap::take` is sound there only because `Plugin` is a zero-sized opaque type, so the `Box` owns no memory.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — a mechanical dead-parameter removal, and the second commit's rewritten SAFETY comment in src/runtime/server/server_body.rs now matches what heap::take actually wraps (an opaque_ffi! zero-sized handle to a protected JSC cell).
Checked that JSBundlerPlugin__create has the same single-argument shape on the C++ extern "C" definition and the Rust unsafe extern "C" declaration, that no file in the repo still references BunPluginTarget or the deleted *Node/*Browser host functions, and that the only remaining target uses in BunPlugin.cpp are the kept string validation and the builder's "bun" property.
Extended reasoning...
The change removes an unread BunPluginTarget value end to end: the C++ typedef and constants in headers-handwritten.h, four target members, the parameter on JSBundlerPlugin::create/constructors/JSBundlerPlugin__create and the BunPlugin.cpp helpers, four never-registered host functions, and the Rust enum plus three call sites. It touches an FFI boundary but both sides of the signature were updated together and a repo-wide search finds no stale consumer; no security-sensitive surface is involved. The one inline finding is a pre-existing leak of the protected plugin cell that this PR neither introduces nor worsens, and the earlier inline comment about the inaccurate SAFETY text was addressed by commit 7e7ce61. No CODEOWNERS entry covers the changed files, and the diff is small (+26/-84) and behavior-preserving.
…ve-plugin-target-value Conflict in `JSBundlerPlugin__create`: keep the null prototype from main, and end the call without the `target` argument.
There was a problem hiding this comment.
LGTM — a mechanical dead-code removal, and the SAFETY-comment correction from the earlier inline thread is now in the branch.
What was reviewed:
JSBundlerPlugin__creatediffed parameter-by-parameter across the Rustunsafe extern "C"declaration and the C++ definition (both now one argument); the post-merge conflict resolution insrc/jsc/bindings/JSBundlerPlugin.cppkeeps the null prototype and ends the call withnullptr.- Repo-wide search for
BunPluginTargetand the four deleted Node/Browser host functions returns nothing;setupBunPluginstill registers theBunpair, and all three Rust callers (JSBundler.rs,bake/config/bundler.rs,server_body.rs) use the new one-argumentPlugin::create. - The rewritten SAFETY comment in
server_body.rschecked againstopaque_ffi!(arepr(C)ZST) andheap::take(Box::from_raw) — accurate; the newLoaderdoc example insrc/opaque/lib.rsmatches the real discriminants insrc/ast/loader.rs.
Extended reasoning...
The change deletes an unread plugin-target value across the C++/Rust FFI boundary: the BunPluginTarget typedef and enum, four target members, the parameter on JSBundlerPlugin__create and five Bun.plugin helpers, and four uncalled host functions, plus a corrected SAFETY comment in server_body.rs. It touches no security-sensitive surface; the only unsafe code affected is the heap::take on a zero-sized opaque JSC handle, whose comment is now accurate. The only commit since the prior review is a merge of origin/main whose single conflict resolution was verified. The change is small, purely subtractive, every removed symbol has zero remaining references, the FFI signature matches on both sides, no CODEOWNERS entry covers the changed files, and no third-party changes-requested review is visible, so a human look is not needed.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
This branch has not been deployed
1 participant
Problem
JSBundlerPlugin__createtakes aBunPluginTargetand stores it inBundlerPlugin::target(src/jsc/bindings/JSBundlerPlugin.h:164). No code reads it, or thetargetmember of three more classes.Bun.pluginhelpers inBunPlugin.cpptake the same type and do not use it. Implement virtual module support inBun.plugin#6167 removed the last reader in 2023.Fix
JSBundlerPlugin__create,JSBundlerPlugin::create, both constructors,PluginJscExt::createand fiveBun.pluginhelpers. Delete the four members, the Rust enum and the C++ typedef.bunpair.bun bd,bun run rust:check-all(12 targets), 8 test files (Notes).Background
JSBundlerPlugincell for the plugins ofBun.build, of the bake config, and of[serve.static].Bun.plugin({ target })still throws aTypeErrorfor a string other thannode,bunorbrowser.Downsides
targettests inplugins.test.ts.Behaviour change: none
Notes
Removed
src/jsc/bindings/JSBundlerPlugin.h: thetargetmembers ofBundlerPlugin,NamespaceListandNativePluginList, the constructor parameter, and the storethis->target = target.src/jsc/bindings/JSBundlerPlugin.cpp: the parameter ofJSBundlerPlugin::create, of the private constructor, and ofJSBundlerPlugin__create.src/jsc/bindings/BunPlugin.h: thetargetmember ofBunPlugin::Group.src/jsc/bindings/BunPlugin.cpp: the parameter ofjsFunctionAppendOnLoadPluginBody,jsFunctionAppendOnResolvePluginBody,jsFunctionAppendOnLoadPluginGlobal,jsFunctionAppendOnResolvePluginGlobalandsetupBunPlugin, and the argument at each call.src/jsc/bindings/BunPlugin.cpp: the host functionsjsFunctionAppendOnLoadPluginNode,jsFunctionAppendOnLoadPluginBrowser,jsFunctionAppendOnResolvePluginNodeandjsFunctionAppendOnResolvePluginBrowser(20 lines). Each name occurs only at its definition, andsetupBunPluginregisters only theBunpair. With the parameter gone, each body is the same line as theBunone. REVIEW.md says to delete one of two functions that a change makes identical.src/jsc/bindings/headers-handwritten.h:BunPluginTarget,BunPluginTargetBun,BunPluginTargetBrowser,BunPluginTargetNode,BunPluginTargetMax.src/runtime/api/JSBundler.rs: theBunPluginTargetenum, itsassert_ffi_discr!and comment, its re-export, the parameter of the extern declaration and ofPluginJscExt::create, and thematch this.targetthat built the argument.src/runtime/bake/config/bundler.rs,src/runtime/server/server_body.rs: the argument at the two other call sites.src/opaque/lib.rs: the doc example ofassert_ffi_discr!namedBunPluginTarget. It now namesLoader, which has a real use of the variant form insrc/ast/loader.rs.Changed
src/runtime/server/server_body.rs: the SAFETY comment below the changed call said thatPlugin::createreturns a boxed pointer. It returns a protected JSC cell, andheap::takeis sound there becausePluginis a zero-sized type. The comment now says that. A review of this pull request found it. The field staysBox<Plugin>.Kept
targetcheck insetupBunPluginand its messageplugin target must be one of 'node', 'bun' or 'browser'. Thetargetproperty of the builder object stays"bun".Bun.buildreads its owntargetoption from the config....Globalhelpers, each with one caller now.jsFunctionAppendVirtualModulehas the same shape, and a fold widens the conflict with Remove dead code from the JSC bindings, headers.h, the console builtin, and bun_jsc #40232.Found unread in the same files, and left
void* configin the same three signatures, withBundlerPlugin::configandJSBundlerPlugin__setConfig. C++ only passes the value back to the callbacks, and Rust names that argument_unused. Bun.build / Bun.Transpiler: remove unsafe from JSBundler, the completion task and JSTranspiler #40405 removes them.vmparameter ofNamespaceList::appendand ofNativePluginList::append, thepluginparameter ofNativePluginList::call, and theindexout-parameter ofNamespaceList::groupandNamespaceList::append. They are not part of thetargetflow, and they stay for a change of their own.History
BunPlugin::Group::target. No commit added a reader. Support plugins inBun.build#2720 (2023-04-24) copied that class tree intoJSBundlerPlugin.h, and Introduce high-performance native addon API inBun.build, starting withbuild.onBeforeParsehook #14971 copiedNamespaceListasNativePluginList. So three of the four members are one line.Bun.build#2720 also addedBundlerPlugin::targetwith the store. No commit added a reader.Bun.plugin#6167 (2023-09-29),Bun.pluginkept one plugin list for each target (onLoadPlugins[target],onResolvePlugins[target]), and aswitch (target)insetupBunPluginregistered the host functions of that target. That pull request made the lists single and left the parameters and the four functions.targetparameter ofBun__runOnLoadPluginsandBun__runOnResolvePlugins, and the enum inbun_bundlerandbun_jsc. It moved the enum toJSBundler.rs.Node = 1, Browser = 2. The C++ constants hadBrowser = 1, Node = 2. A reader on the C++ side would have got the wrong target.Merge with main
JSBundlerPlugin__create. The merge commit a81985a keeps its null prototype (jsNull()),) and ends the call withnullptr);.Open pull requests
git merge-tree) shows that six gain conflict hunks, 12 in total. Each of the six already conflicts with main in other files.configfrom the same constructor lines.BunPluginTargetMax, which must go with the type.bun build --compile --aot#45631 (2 hunks): new code directly above a changed signature inBunPlugin.cpp.Tests, debug build of this branch after the merge with main
test/js/bun/plugin/plugins.test.ts: 110 pass. This includeshandles invalid 'target'and the two tests of atargetthat throws during string conversion.test/js/bun/plugin/plugin-namespace-drive-letter.test.ts: 1 pass.test/bundler/bundler_plugin.test.ts: 67 pass, 3 todo.test/bundler/native-plugin.test.ts: 19 pass, 1 skip (with--timeout 1200000, because thebeforeAllinstalls and compiles a native addon).test/bake/serve-plugins-dev-server.test.ts: 11 pass.test/bake/dev/plugins.test.ts: 3 pass.test/bake/app-options.test.ts: 13 pass.test/bundler/bun-build-api.test.ts: 87 pass, 1 skip, 1 todo, 3 timeouts. The two bytecode tests pass with--timeout 900000.Bun.build can be called thousands of times in one process without crashingexceeds its own 180 s limit with a debug build in this container. None of the three uses a plugin.no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check