Repository navigation
Conversation
build-fallbacks.ts minifies each polyfill with --minify-syntax. That pass drops the name of a function expression that nothing refers to. So in a `bun build --target=browser` bundle, `EventEmitter.prototype.once.name` is "". Node gives "once", and so did Bun 1.3.1. Pass --keep-names to the polyfill build, as bundle-modules.ts and bundle-functions.ts do. The build fails if an output contains a __name() call. When a bundle has the exports object of a polyfill, the bundler numbers each function expression that has the name of an export of that polyfill. EventEmitter is now a function declaration, and http.js exports `get` through a binding with another name. So `EventEmitter.name` and the getters of stream-http keep the names that they have today.
|
No actionable comments were generated in the recent review. WalkthroughThe fallback build now enables function-name preservation and checks for ChangesBrowser polyfill function names
Possibly related PRs
Suggested reviewers: Merge Risk:
|
|
Status: ready for review. Reproduced on bun 1.4.3 and on main 447ff87 with this file: // names.mjs
import { EventEmitter } from "events";
console.log(JSON.stringify([EventEmitter.prototype.on.name, EventEmitter.prototype.once.name, EventEmitter.prototype.removeListener.name]));
PR: #48629 |
…ough other bindings The bundler numbers a function expression that has the name of an export when the bundle has the exports object of the polyfill. The four EventEmitter.prototype methods and util's inspect and promisify read `once2`, `inspect2` and so on in that case. Export the six names through bindings with other names, as http.js does for `get`. The comments are one line each.
There was a problem hiding this comment.
I re-reviewed after 1aabe2d and found no bugs; the aliased once/listenerCount/setMaxListeners/getMaxListeners/inspect/promisify exports and the shortened comments address what I raised on events.js and the comment style. Because --keep-names changes the embedded polyfill bytes in every release binary and the output size of every browser bundle (crypto.js alone +8 KB of text), a maintainer should still weigh that size-for-names tradeoff.
What was reviewed:
- events.js/util.js aliasing: the four events names are hoisted function declarations and util's
inspect/promisifyconsts are declared above the alias lines, so no TDZ or shadowing; util's default export object is unchanged. - The
__name(guard in build-fallbacks.ts: no tracked polyfill source contains that text, so it can only trip on bundler output. - The new
itBundledcases usebackend,keepNames, and.optionsas expectBundled.ts defines them; they assert exact stdout. - Not run:
USE_SYSTEM_BUN=1 bun testto confirm the five new tests fail without the build change, andbun bd testto confirm they pass.
Extended reasoning...
The change adds --keep-names to the bun build invocation in src/node-fallbacks/build-fallbacks.ts that prebuilds the 23 browser polyfills embedded in release binaries, adds a post-build guard against __name() helpers, restructures events.js, http.js and util.js so exports whose names collide with function expressions go through aliased bindings, and adds five itBundled cases in test/bundler/bundler_browser.test.ts. It touches no security-sensitive surface. The follow-up commit addressed two of the three optional inline comments from the prior run; the PATH-resolved bun concern in build-fallbacks.ts predates this PR and remains as an open thread. Deferring rather than approving because the flag changes release artifact size and every user's browser bundle output, which is a product tradeoff for a maintainer, and because the fail-without-fix test check could not be made by reading.
Problem
bun build --target=browserbundle,EventEmitter.prototype.once.nameis"". Node and Bun 1.3.1 give"once". No user reported it.src/node-fallbacks/build-fallbacks.ts:36minifies each polyfill with--minify-syntax, which drops unused function expression names since Remove unused function/class names when minifying #22492. ci: update bun version to 1.3.1 #24053 moved the build to a bun that has it.Fix
--keep-namesto the polyfill build, asbundle-modules.tsandbundle-functions.tsdo. The build fails if an output calls__name()(bundler: implement --keep-names with __name helper #40503). Minified bundles keep today's names.EventEmitteris a function declaration, andevents.js,util.jsandhttp.jsexport 7 functions through other bindings. The flag alone prints them asEventEmitter2,once2,get2. They can go when the bundler stops numbering.test/bundler/bundler_browser.test.ts(5 new tests fail on Bun 1.4.3). Other suites: Notes.Background
src/node-fallbacks/*.jsreplace Node builtins in a browser build. The bun that builds bun bundles them. Releases embed the result.require("events")gives the bundle the module's exports object. The bundler then numbers function expressions named like an export, in user modules too.--minify-syntax(+39,774 B), orObject.definePropertyname lines (one call per name).Downsides
eventsbundle +170 B (+3 B with--minify).Uint8Array.fillfor aBufferframe (Buffer.filltoday).movper call of a function with a kept name (emit: 112 to 113 bytecodes).Notes
How it was found. Node's
test/parallel/test-event-emitter-method-names.jsassertsE[name].name === namefor eachEventEmitter.prototypemethod. Run by hand against a default browser bundle, 13 of 13 checks fail (emitreadsemitWithoutRejectionCapture). Bun's runtime passes that test. #48481 lists the gap under "Not covered". There is no issue and no user report.History (release binaries,
bun build --target=browser, named imports fromnode:events):EventEmitter.nameonce,listenerCountexportsEventEmitter2once2,listenerCount2EventEmitteronce2,listenerCount2""EventEmitter""EventEmitterOnly the prototype method names are a regression from 1.3.1. No file under
src/node-fallbackschanged between 1.3.1 and 1.3.2. #22492 (2025-09-09) added the name removal to--minify-syntax, with--keep-namesas the opt-out.bundle-modules.tsandbundle-functions.tsgotkeepNameson 2025-10-18 (e9e9ca4, bd15fce). #24053 (2025-10-25) moved the build to bun 1.3.1, andbuild-fallbacks.tsdid not have the flag.What the user's flags give (the file above):
--minify-whitespace""--minify-syntax --keep-names""--minify-syntax,--minify,--production""""--minify-identifiers,--minify --keep-names""Polyfill text (
wc -cof the 23 outputs ofbuild-fallbacks.ts. Host bun 1.4.4, the CI pin, and 1.4.3 write identical files.):events.js,util.jsandhttp.jsdiffer (the declaration and the export bindings).__name(in the 23 outputs: 0. No call is added at module evaluation. The 7 export bindings are plain copies, and a bundle that does not use them drops them.Release binary (
readelf -Sandsize -Aon two release builds in one build directory that differ only in the polyfill outputs.bloatyis not installed.):.rodata27,239,404 to 27,243,500 B (+4,096), file 143,284,496 to 143,288,592 B (+4,096). The zstd data round up to one 4 KiB page. No other section changes size. These two builds are from before the export bindings inevents.jsandutil.js, which add 54 B of zstd data.User bundles (debug build of main, main's polyfill text against this PR's, bytes of the output):
--minify-whitespace--minify-syntax--minify-syntax --keep-names--minify-identifiers--minify--minify --keep-names--minify-syntax--minify-identifiers--minifyimport * as m from "node:crypto"--minify--minify--minify--minify--minify--minify--minifyNames against node v26.3.0. A script walks
.nameof each function that the 23 polyfills export, to depth 3, forimport * asand forrequire(). 964 of those paths also exist in node.Buffer.prototype.readUInt8and 14 more read methods and aliases,crypto.privateDecryptand 3 more). Bun's own runtime names them too.Buffer.from,Buffer.alloc,Buffer.allocUnsafe).bun buildbundling of CJSrequire("node:assert")does not properly expose exports. and returns an empty{}object #19808 is not changed.ok2): 73 paths on main, the same 73 with this PR (assert 62, querystring 11).Stack frames. The kept names also show in stack traces of unminified bundles. JSC:
at <anonymous>becomesat fill. V8:at EventEmitter.emitWithoutRejectionCapture [as emit]becomesat EventEmitter.emit, as in node. ForBuffermethods V8 printsat Uint8Array.fillwhere it printsat Buffer.filltoday.The numbered names.
src/bundler/linker_context/doStep5.rs:626gives each export in an exports object a use with no scope.src/js_printer/renamer.rs:1345then treats that symbol as seen in every nested scope, so a function expression with the same name gets a number. This PR does not change the bundler.export function once() {}witho.once = function once() {}printsonce2behindimport * as.assert.rejects(Promise.resolve(1))in a browser bundle rejects withMissing expected exception.andoperator: "rejects2". Node and Bun 1.4.0 giveMissing expected rejection.and"rejects". This PR's text gives the same result as main's.EventEmitter2(right on main),get2forhttp.getand 13 getters of stream-http (the getters are right on main), andonce2,listenerCount2,getMaxListeners2,setMaxListeners2,inspect2,promisify2(""on main).EventEmitter2shows in each bundle that hasnode:stream.EventEmitteris a declaration, which has one binding. The other 7 are exported through a second binding (var httpGet = http.get; export { httpGet as get }), so the export and the function expression do not share a name. When the bundler stops numbering these, the bindings can be removed. The new tests do not depend on them..name, even with--keep-names#44121 is a different cause: two modules that declare one top-level name.JSC bytecode (
BUN_JSC_dumpGeneratedBytecodes=1, a bundle that callson,once,emit,removeListener):emit112 to 113 instructions,addListener58 to 59,once21 to 22,removeListener51 to 52,listenerCount14 to 15. The added instruction ismov loc, calleeat function entry.perfandvalgrindare not installed, so machine instructions are not measured.#40503. That PR makes
--keep-nameswrap functions in__name()calls, and--minify-syntaxthen drops the inline names again. With a host bun that has it, the new check fails this build. The build that keeps the names inline is then--minify-whitespacealone: 28,774 B more text than this PR (2,692 B zstd).bundle-modules.ts:240passes the same two flags, so both scripts need that decision together.Other open PRs. None of them keeps the names, and this PR does not wait for them.
Bun.build(). The option there isminify.keepNames.keepNamesadds__name()calls, which the new check stops. The change that fits there is nominifySyntax.module.exportsexport from require() of an ES module #42092 adds an export line toevents.jsandhttp.js. node-fallbacks: emit "removeListener" from removeListener() in the events polyfill #48481 edits other lines ofevents.js.Self-review. 22 concerns. 4 ask for the bundler change above to land first, as its own PR, and for this PR to drop the source edits then. That change is not part of this PR. The edits stay until it exists, and the section above gives the cause. 1 asks to wait for #41524, #41533 and #40503. The other 17 changed the check's message, the tests and this text.
Tests.
bun bd test test/bundler/bundler_browser.test.ts: 28 pass. WithUSE_SYSTEM_BUN=1(1.4.3) the 5 new tests fail. Without the flag 5 fail. With the named function expression back inevents.js2 fail (EventEmitter2). Without the export bindings ofhttp.js,events.jsorutil.js1 fails for each (get2,once2,inspect2). Also on the debug build:bundler_regressions(9 pass),esbuild/packagejson(94 pass),bundler_plugin(68 pass),bundler_splitting(209 pass),bundler_minify -t KeepNames(3 pass).bundler_edgecase,esbuild/defaultandbun-build-apipass except 5 tests that miss their deadlines on a debug build (DeepImportDiamondDAG,TopLevelAwaitForbiddenRequireSourceMapCLI and API, twoBun.buildstress tests). They bundle no polyfill, and the first three fail the same way with main's polyfill text.