Repository navigation
node:url: do not emit DEP0169 in a standalone executable #46210
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -0,0 +1,198 @@ | ||
| import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isASAN, isDebug, isWindows, tempDir } from "harness"; | ||
| import { join } from "node:path"; | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/16639 | ||
| // | ||
| // url.parse() prints DEP0169 unless the file of its caller is under node_modules. Without | ||
| // --sourcemap every frame of a standalone executable has the name of the executable. With | ||
| // --sourcemap the names are relative to the build directory ("node_modules/dep/index.js"), | ||
| // and the check needs a separator before "node_modules". In both builds the call of a bundled | ||
| // dependency printed the warning (axios before 1.14.0, engine.io before 6.6.5, parseurl 1.3.3). | ||
|
|
||
| // url.parse() and the four other node:url entry points that go through the same check. | ||
| const urlCalls = /* js */ ` | ||
| const url = require("node:url"); | ||
| module.exports = { | ||
| "url.parse": () => url.parse("http://example.com/a"), | ||
| "url.format": () => url.format("http://example.com/a"), | ||
| "url.resolve": () => url.resolve("http://example.com/a", "b"), | ||
| "url.resolveObject": () => url.resolveObject("http://example.com/a", "b"), | ||
| "Url.prototype.resolve": () => { | ||
| const parsed = new url.Url(); | ||
| parsed.parse("http://example.com/a"); | ||
| return parsed.resolve("b"); | ||
| }, | ||
| }; | ||
| `; | ||
|
|
||
| const files = { | ||
| "package.json": JSON.stringify({ name: "app", version: "1.0.0" }), | ||
| "node_modules/dep/package.json": JSON.stringify({ name: "dep", version: "1.0.0", main: "index.js" }), | ||
| "node_modules/dep/index.js": urlCalls, | ||
| "own.js": urlCalls, | ||
| // Each argument is one step: "dep" and "own" make every call from that file, | ||
| // "worker" makes the calls of "dep" in a Worker, "emit" emits another deprecation. | ||
| "app.js": /* js */ ` | ||
| const { Worker } = require("node:worker_threads"); | ||
| const callers = { dep: require("dep"), own: require("./own.js") }; | ||
| (async () => { | ||
| for (const step of process.argv.slice(2)) { | ||
| if (step === "emit") { | ||
| process.emitWarning("still printed", "DeprecationWarning", "DEP9999"); | ||
| } else if (step === "worker") { | ||
| await new Promise((resolve, reject) => { | ||
| const worker = new Worker("./worker.js"); | ||
| worker.on("message", resolve); | ||
| worker.on("error", reject); | ||
| }); | ||
| } else { | ||
| for (const name in callers[step]) callers[step][name](); | ||
| } | ||
| } | ||
| console.log("done"); | ||
| })(); | ||
| `, | ||
| "worker.js": /* js */ ` | ||
| const { parentPort } = require("node:worker_threads"); | ||
| const dep = require("dep"); | ||
| for (const name in dep) dep[name](); | ||
| parentPort.postMessage("done"); | ||
| `, | ||
| }; | ||
|
|
||
| // A standalone executable of a debug or ASAN build is over 1 GB. It is slow to write and to start. | ||
| const timeout = isDebug || isASAN ? 60_000 : undefined; | ||
|
Comment on lines
+64
to
+65
Contributor
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. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove the explicit test and hook timeouts. Line 65 defines a per-test Remove the Proposed change-// A standalone executable of a debug or ASAN build is over 1 GB. It is slow to write and to start.
-const timeout = isDebug || isASAN ? 60_000 : undefined;
- beforeAll(async () => {
dir = tempDir("issue-16639", files);
- // One at a time: two writes of a debug executable at once take longer than one after the other.
exe = await compile("app", []);
exeWithSourcemap = await compile("app-sourcemap", ["--sourcemap"]);
- }, 180_000);
+ });Remove the trailing As per coding guidelines: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." Also applies to: 100-100 🤖 Prompt for AI AgentsSource: Coding guidelines
Contributor
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. 🟡 nit (optional): Maintainers get a test that sets its own timeouts, which test/CLAUDE.md forbids ("CRITICAL: Do not set a timeout on tests. Bun already has timeouts"). sweep: Why this was flaggedtest/CLAUDE.md (section "No timeouts") says not to set a timeout on tests because the runner already has one. test/regression/issue/16639.test.ts:65 defines Verification: nit. test/CLAUDE.md states at lines 118-120: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The new file breaks it: test/regression/issue/16639.test.ts:65 |
||
|
|
||
| describe("url.parse() deprecation warning (DEP0169)", () => { | ||
| let dir: ReturnType<typeof tempDir>; | ||
| let exe: string; | ||
| let exeWithSourcemap: string; | ||
|
|
||
| async function run(cmd: string[], env: Record<string, string> = {}) { | ||
| await using proc = Bun.spawn({ | ||
| cmd, | ||
| env: { ...bunEnv, ...env }, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| return { stdout, stderr, exitCode }; | ||
| } | ||
|
|
||
| const countOfDEP0169 = (stderr: string) => stderr.split("[DEP0169]").length - 1; | ||
| const quiet = { stdout: "done\n", stderr: "", exitCode: 0 }; | ||
|
|
||
| async function compile(name: string, flags: string[]) { | ||
| const outfile = join(String(dir), isWindows ? `${name}.exe` : name); | ||
| const cmd = [bunExe(), "build", "--compile", ...flags, "./app.js", "./worker.js", "--outfile", outfile]; | ||
| const build = await run(cmd); | ||
| if (build.exitCode !== 0) throw new Error(`bun build --compile failed:\n${build.stderr}`); | ||
| return outfile; | ||
| } | ||
|
|
||
| beforeAll(async () => { | ||
| dir = tempDir("issue-16639", files); | ||
| // One at a time: two writes of a debug executable at once take longer than one after the other. | ||
| exe = await compile("app", []); | ||
| exeWithSourcemap = await compile("app-sourcemap", ["--sourcemap"]); | ||
| }, 180_000); | ||
|
|
||
| afterAll(() => dir?.[Symbol.dispose]()); | ||
|
|
||
| describe("in a standalone executable", () => { | ||
|
Contributor
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. 🟡 (optional) Windows CI maintainers can see afterAll throw Why this was flaggedEvery Verification: nit — triggers only on Windows when a |
||
| test.concurrent( | ||
| "the calls of a dependency print nothing", | ||
| async () => { | ||
| expect(await run([exe, "dep"])).toEqual(quiet); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| test.concurrent( | ||
| "the calls of the program print nothing", | ||
| async () => { | ||
| expect(await run([exe, "own"])).toEqual(quiet); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| test.concurrent( | ||
| "the calls of a dependency in a Worker print nothing", | ||
| async () => { | ||
| expect(await run([exe, "worker"])).toEqual(quiet); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| // The frames have the names of their sources here: "node_modules/dep/index.js" and "own.js". | ||
| test.concurrent( | ||
| "the calls print nothing when the executable has a sourcemap", | ||
| async () => { | ||
| expect(await run([exeWithSourcemap, "dep", "own"])).toEqual(quiet); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| test.concurrent( | ||
| "the calls print nothing and do not throw with --throw-deprecation", | ||
| async () => { | ||
| const options = "--throw-deprecation --trace-deprecation --pending-deprecation"; | ||
| expect(await run([exe, "dep", "own"], { BUN_OPTIONS: options })).toEqual(quiet); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| test.concurrent( | ||
| "other deprecation warnings are still printed", | ||
| async () => { | ||
| const { stdout, stderr, exitCode } = await run([exe, "dep", "emit"]); | ||
| expect(stderr).toContain("[DEP9999] DeprecationWarning: still printed"); | ||
| expect({ stdout, DEP0169: countOfDEP0169(stderr), exitCode }).toEqual({ | ||
| stdout: "done\n", | ||
| DEP0169: 0, | ||
| exitCode: 0, | ||
| }); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| // BUN_BE_BUN=1 makes the executable the bun CLI: it reads app.js from the disk. | ||
| test.concurrent( | ||
| "the executable as the bun CLI prints the warning", | ||
| async () => { | ||
| const { stdout, stderr, exitCode } = await run([exe, "app.js", "dep", "own", "own"], { BUN_BE_BUN: "1" }); | ||
| expect({ stdout, DEP0169: countOfDEP0169(stderr), exitCode }).toEqual({ | ||
| stdout: "done\n", | ||
| DEP0169: 1, | ||
| exitCode: 0, | ||
| }); | ||
| }, | ||
| timeout, | ||
| ); | ||
| }); | ||
|
|
||
| describe("with bun run", () => { | ||
| test.concurrent( | ||
| "the calls of a dependency print nothing", | ||
| async () => { | ||
| expect(await run([bunExe(), "app.js", "dep"])).toEqual(quiet); | ||
| }, | ||
| timeout, | ||
| ); | ||
|
|
||
| test.concurrent( | ||
| "the calls of the program print the warning one time", | ||
| async () => { | ||
| const { stdout, stderr, exitCode } = await run([bunExe(), "app.js", "dep", "own", "own"]); | ||
| expect({ stdout, DEP0169: countOfDEP0169(stderr), exitCode }).toEqual({ | ||
| stdout: "done\n", | ||
| DEP0169: 1, | ||
| exitCode: 0, | ||
| }); | ||
| }, | ||
| timeout, | ||
| ); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) Developers who run a compiled executable with an explicit deprecation flag get no DEP0169 for their own
url.parse()calls, where the base branch warns or throws. src/js/node/url.ts:128 seedsurlParseWarnedtoBun.isStandaloneExecutable, so the check at url.ts:138 never fires in an executable even under--throw-deprecation,--trace-deprecationor--pending-deprecation. The PR calls this an accepted downside with no opt-in; it follows from the stated purpose but removes the only audit path for first-party callers. Fix: stay quiet by default in executables but honor an explicit opt-in, e.g. seed withBun.isStandaloneExecutable && !process.pendingDeprecation(or also!process.throwDeprecation), and flip the 16639.test.ts assertion that--pending-deprecationprints nothing.Why this was flagged
A developer builds
bun build --compile app.jswhere app.js itself callsurl.parse()and runs the result asBUN_OPTIONS=--throw-deprecation ./appin CI to catch deprecated API use. On the base branch url.ts:138 runsisInsideNodeModules(4), the frame name is/$bunfs/root/app, so DEP0169 is emitted and with--throw-deprecationthe process exits 1. After this change src/js/node/url.ts:128 setsurlParseWarnedtotrueat module load becauseBun.isStandaloneExecutableis true, so theif (!urlParseWarned && ...)at url.ts:138 is never entered andprocess.emitWarningis never reached regardless of flags.process.pendingDeprecation/process.throwDeprecationare available (src/jsc/bindings/BunProcess.cpp:5157-5162) but nothing in url.ts consults them. test/regression/issue/16639.test.ts:138-145 pins this: it asserts that with--throw-deprecation --trace-deprecation --pending-deprecationthe executable prints nothing and exits 0. The consequence is a lost diagnostic for first-party code in executables with no switch to restore it.Verification: src/js/node/url.ts:128
let urlParseWarned = Bun.isStandaloneExecutable;, so the guard at url.ts:138 is dead in every executable realm regardless of CLI flags. On the base branchurlParseWarned = false, so first-party calls warned and, under--throw-deprecation, the process exited nonzero. test/regression/issue/16639.test.ts:138-145 pins{stdout:"done\n", stderr:"", exitCode:0}for first-party calls.