Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions src/bun_core/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1991,6 +1991,13 @@ pub fn pretty_fmt_runtime(fmt: &[u8], is_enabled: bool) -> Vec<u8> {
i += 1;
}
b'{' => {
// `{{` is an escaped literal brace, not the start of a
// placeholder: the text after it is still subject to markup.
if fmt.get(i + 1) == Some(&b'{') {
out.extend_from_slice(b"{{");
i += 2;
continue;
}
while i < fmt.len() && fmt[i] != b'}' {
out.push(fmt[i]);
i += 1;
Expand Down Expand Up @@ -2940,6 +2947,55 @@ mod pretty_fmt_tests {
assert_eq!(pretty_fmt!("{?}", true), "{:?}");
}

#[test]
fn markup_after_escaped_brace_is_rewritten() {
// `{{` is a literal brace, not the start of a spec: tags between it
// and the next `}` must still be converted / stripped.
assert_eq!(
pretty_fmt!(
"<r><green>\"{}\"<r>: {{\n <green>\"bun\"<r>: <green>\"latest\"<r>\n}}",
false
),
"\"{}\": {{\n \"bun\": \"latest\"\n}}"
);
assert_eq!(pretty_fmt!("{{<green>x<r>}}", true), "{{\x1b[32mx\x1b[0m}}");
assert_eq!(pretty_fmt!("{{<green>x<r>}}", false), "{{x}}");
assert_eq!(pretty_fmt!("{{{{<r>", false), "{{{{");
// The escapes pass through untouched, so the output is still a valid
// `format_args!` template, including when an escape abuts a spec.
assert_eq!(
format!(pretty_fmt!("{{ <green>{s}<r> }}", false), "bun"),
"{ bun }"
);
assert_eq!(pretty_fmt!("<b>{{{s}}}<r>", false), "{{{}}}");
assert_eq!(format!(pretty_fmt!("<b>{{{s}}}<r>", false), "bun"), "{bun}");
// A spec after the escape is still shielded from the tag parser.
assert_eq!(pretty_fmt!("{{<d>{:<4}<r>}}", false), "{{{:<4}}}");
}

#[test]
fn runtime_markup_after_escaped_brace_is_rewritten() {
assert_eq!(
pretty_fmt_runtime(b"{{<green>x<r>}}", true),
b"{{\x1b[32mx\x1b[0m}}"
);
assert_eq!(pretty_fmt_runtime(b"{{<green>x<r>}}", false), b"{{x}}");
assert_eq!(
format!(
"{}",
super::pretty_fmt_args("{{ <green>{s}<r> }}", false, ("bun",))
),
"{ bun }"
);
assert_eq!(
format!(
"{}",
super::pretty_fmt_args("{{ <green>{s}<r> }}", true, ("bun",))
),
"{ \x1b[32mbun\x1b[0m }"
);
}

#[test]
fn concat_and_stringify_inputs() {
assert_eq!(
Expand Down Expand Up @@ -3008,5 +3064,7 @@ mod pretty_fmt_tests {
check!(
" <b><blue>link<r> <d>[\\<package\\>]<r> Register or link a local npm package\n"
);
check!("<r><green>\"{}\"<r>: {{\n <green>\"bun\"<r>: <green>\"latest\"<r>\n}}");
check!("{{{{<r>{{<d>{:<4}<r>}}");
}
}
7 changes: 7 additions & 0 deletions src/bun_core_macros/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ fn rewrite(fmt: &str, is_enabled: bool) -> Result<String, String> {
i += 1;
}
b'{' => {
// `{{` is `format_args!`'s escaped literal brace, not the start
// of a spec: the text after it is still subject to markup.
if bytes.get(i + 1) == Some(&b'{') {
out.push_str("{{");
i += 2;
continue;
}
// copy `{ ... }` verbatim, optionally rewriting legacy specs
let start = i;
while i < bytes.len() && bytes[i] != b'}' {
Expand Down
5 changes: 5 additions & 0 deletions src/clap_macros/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,11 @@ fn pretty_rewrite(fmt: &[u8], is_enabled: bool) -> Vec<u8> {
i += 1;
}
b'{' => {
if fmt.get(i + 1) == Some(&b'{') {
out.extend_from_slice(b"{{");
i += 2;
continue;
}
while i < fmt.len() && fmt[i] != b'}' {
out.push(fmt[i]);
i += 1;
Expand Down
4 changes: 2 additions & 2 deletions test/cli/install/__snapshots__/bun-install.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ exports[`should report error on invalid format for dependencies 1`] = `
^
error: dependencies expects a map of specifiers, e.g.
"dependencies": {
<green>"bun"<r>: <green>"latest"<r>
"bun": "latest"
}
at [dir]/package.json:1:33
"
Expand Down Expand Up @@ -49,7 +49,7 @@ exports[`bun-install should report error on invalid format for dependencies 1`]
^
error: dependencies expects a map of specifiers, e.g.
"dependencies": {
<green>"bun"<r>: <green>"latest"<r>
"bun": "latest"
}
at [dir]/package.json:1:33
"
Expand Down
49 changes: 48 additions & 1 deletion test/cli/install/bun-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
bunEnv as env,
isWindows,
joinP,
normalizeBunSnapshot,
readdirSorted,
runBunInstall,
tempDir,
Expand Down Expand Up @@ -4965,7 +4966,7 @@ describe.concurrent("bun-install", () => {
` ^`,
`error: optionalDependencies expects a map of specifiers, e.g.`,
` "optionalDependencies": {`,
` <green>"bun"<r>: <green>"latest"<r>`,
` "bun": "latest"`,
` }`,
` at [dir]/package.json:1:33`,
]);
Expand All @@ -4975,6 +4976,52 @@ describe.concurrent("bun-install", () => {
});
});

it("should report error on non-string specifier in dependencies", async () => {
using dir = tempDir("invalid-dependency-specifier", {
"package.json": JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { bun: 1 } }),
});

await using proc = spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(`
"1 | {"name":"foo","version":"0.0.1","dependencies":{"bun":1}}
^
error: dependencies expects a map of specifiers, e.g.
"dependencies": {
"bun": "latest"
}
at <dir>/package.json:1:55"
`);
expect(stdout).toContain("bun install v1.");
Comment on lines +4991 to +5002

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Remove the version-prefix assertion.

The bun install version header is not part of this regression and can change independently. Keep proc.stdout.text() in the concurrent Promise.all, but do not assert its contents. The normalized stderr snapshot and exitCode already verify the invalid-specifier path.

Suggested change
-    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

     expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(`
...
-    expect(stdout).toContain("bun install v1.");
     expect(exitCode).toBe(1);

As per coding guidelines: “Subprocess tests must drain stdout, stderr, and process exit concurrently.”
Based on learnings: “Do not assert stdout for commands like bun install whose output can vary between runs.”

📝 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.

Suggested change
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(`
"1 | {"name":"foo","version":"0.0.1","dependencies":{"bun":1}}
^
error: dependencies expects a map of specifiers, e.g.
"dependencies": {
"bun": "latest"
}
at <dir>/package.json:1:55"
`);
expect(stdout).toContain("bun install v1.");
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(`
"1 | {"name":"foo","version":"0.0.1","dependencies":{"bun":1}}
^
error: dependencies expects a map of specifiers, e.g.
"dependencies": {
"bun": "latest"
}
at <dir>/package.json:1:55"
`);
🤖 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 `@test/cli/install/bun-install.test.ts` around lines 4991 - 5002, Remove the
stdout content assertion following the concurrent Promise.all in the install
test, while retaining proc.stdout.text() in that Promise.all to drain stdout.
Keep the normalized stderr snapshot and exitCode assertions unchanged.

Sources: Coding guidelines, Learnings

expect(exitCode).toBe(1);
});

it("should color the example in the invalid dependencies error when colors are enabled", async () => {
using dir = tempDir("invalid-dependencies-color", {
"package.json": JSON.stringify({ name: "foo", version: "0.0.1", dependencies: [] }),
});

await using proc = spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env: { ...env, FORCE_COLOR: "1" },
stdout: "ignore",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);

expect(stderr).toContain(`\n \x1b[32m"bun"\x1b[0m: \x1b[32m"latest"\x1b[0m\n`);
expect(stderr).not.toContain("<green>");
expect(exitCode).toBe(1);
});

test.serial("should report error on invalid format for workspaces", async () => {
await withContext(defaultOpts, async ctx => {
await writeFile(
Expand Down