Skip to content
Open
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
31 changes: 0 additions & 31 deletions src/ast/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,37 +337,6 @@ impl Expr {
// Expr — property/object/string accessor methods.

impl Expr {
pub fn has_any_property_named(&self, names: &'static [&'static [u8]]) -> bool {
if let Data::EObjectJSON(obj) = &self.data {
return obj
.get()
.properties()
.iter()
.any(|p| bun_core::eql_any_comptime(p.key.slice(), names));
}
let Data::EObject(obj) = &self.data else {
return false;
};
if obj.properties.len_u32() == 0 {
return false;
}

for prop in obj.properties.slice() {
if prop.value.is_none() {
continue;
}
let Some(key) = &prop.key else { continue };
let Data::EString(key_str) = &key.data else {
continue;
};
if bun_core::eql_any_comptime(&key_str.data, names) {
return true;
}
}

false
}

// toJS alias deleted — `to_js` lives in `bun_js_parser_jsc::expr_jsc` extension trait.

/// Don't use this if you care about performance.
Expand Down
7 changes: 0 additions & 7 deletions src/parsers/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1952,13 +1952,6 @@ mod tests {
root.get_object(b"name").is_some()
)
.unwrap();
writeln!(
out,
"has_any={} {}",
root.has_any_property_named(&[b"zzz", b"private"]),
root.has_any_property_named(&[b"zzz"])
)
.unwrap();

let deps = root.get(b"deps").unwrap();
assert!(deps.is_object());
Expand Down
45 changes: 21 additions & 24 deletions src/runtime/cli/init_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,42 +701,39 @@ impl InitCommand {
true,
);

let dependency_groups =
bun_install_types::DependencyGroup::FOUR.map(|group| object.get(group.prop));
// `bun install` does not install every peer, so a peer counts only when the new entry is a peer too.
let declares_dependency = |name: &[u8], target: &[u8]| {
bun_install_types::DependencyGroup::FOUR
.iter()
.zip(dependency_groups)
.any(|(group, listed)| {
(group.prop == target || group.prop != b"peerDependencies")
&& listed.is_some_and(|listed| listed.get(name).is_some())
})
};

let needs_dependencies = 'brk: {
if let Some(deps) = object.get(b"dependencies") {
for (i, dep) in dependencies.iter().enumerate() {
if deps.get(dep.name).is_some() {
needed_dependencies.unset(i);
}
for (i, dep) in dependencies.iter().enumerate() {
if declares_dependency(dep.name, b"dependencies") {
needed_dependencies.unset(i);
}
}
break 'brk needed_dependencies.count() > 0;
};

let needs_dev_dependencies = 'brk: {
if let Some(deps) = object.get(b"devDependencies") {
for (i, dep) in dev_dependencies.iter().enumerate() {
if deps.get(dep.name).is_some() {
needed_dev_dependencies.unset(i);
}
for (i, dep) in dev_dependencies.iter().enumerate() {
if declares_dependency(dep.name, b"devDependencies") {
Comment thread
robobun marked this conversation as resolved.
needed_dev_dependencies.unset(i);
}
}
break 'brk needed_dev_dependencies.count() > 0;
};

let needs_typescript_dependency = !minimal
&& 'brk: {
if let Some(deps) = object.get(b"devDependencies") {
if deps.has_any_property_named(&[b"typescript"]) {
break 'brk false;
}
}
if let Some(deps) = object.get(b"peerDependencies") {
if deps.has_any_property_named(&[b"typescript"]) {
break 'brk false;
}
}
true
};
let needs_typescript_dependency =
!minimal && !declares_dependency(b"typescript", b"peerDependencies");

need_run_bun_install =
needs_dependencies || needs_dev_dependencies || needs_typescript_dependency;
Expand Down
100 changes: 100 additions & 0 deletions test/cli/init/init.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,106 @@ const initEnv = { ...bunEnv, BUN_AGENT_RULE_DISABLED: "1" };
});
}, 30_000);

// `bun init` leaves these fields of a package.json as they are, so only a dependency can change.
const configured = {
name: "pkg",
module: "index.ts",
type: "module",
private: true,
scripts: { typecheck: "bun check" },
};

// Runs `bun init` over a package.json. The registry records each request of the nested `bun install`.
async function initOverPackageJson(manifest: object, args: string) {
const requests: string[] = [];
using registry = Bun.serve({
port: 0,
fetch(request) {
requests.push(decodeURIComponent(new URL(request.url).pathname));
return new Response(null, { status: 404 });
},
});
await using temp = tempDir("bun-init-listed-dependency", {
"package.json": JSON.stringify(manifest, null, 2) + "\n",
"index.ts": "export {};\n",
});

await using proc = Bun.spawn({
cmd: [bunExe(), "init", ...args.split(" ")],
cwd: temp,
stdio: ["ignore", "pipe", "pipe"],
env: {
...initEnv,
BUN_CONFIG_REGISTRY: registry.url.href,
BUN_INSTALL_CACHE_DIR: path.join(temp, ".bun-cache"),
},
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

return { manifest: await Bun.file(path.join(temp, "package.json")).json(), requests, stdout, stderr, exitCode };
}

test.each([
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{
args: "-y",
where: "dependencies lists both packages",
groups: { dependencies: { "@types/bun": "1.1.0", typescript: "5.6.2" } },
},
{
args: "-y",
where: "optionalDependencies lists both packages",
groups: { optionalDependencies: { "@types/bun": "1.1.0", typescript: "5.6.2" } },
},
{
args: "-m -y",
where: "dependencies lists both packages",
groups: { dependencies: { "@types/bun": "1.1.0", typescript: "5.6.2" } },
},
{
// These are the groups that `bun init` writes to.
args: "-y",
where: "devDependencies lists @types/bun and peerDependencies lists typescript",
groups: { devDependencies: { "@types/bun": "1.1.0" }, peerDependencies: { typescript: "5.6.2" } },
},
])("bun init $args adds no dependency when $where", async ({ args, groups }) => {
const manifest = { ...configured, ...groups };

const after = await initOverPackageJson(manifest, args);

expect({ manifest: after.manifest, requests: after.requests }).toEqual({ manifest, requests: [] });
expect(after).toMatchObject({ exitCode: 0 });
});

test.each([
{
args: "-y",
where: "dependencies lists typescript",
groups: { dependencies: { typescript: "5.6.2" } },
added: { devDependencies: { "@types/bun": "latest" } },
},
{
args: "-y",
where: "dependencies lists @types/bun",
groups: { dependencies: { "@types/bun": "1.1.0" } },
added: { peerDependencies: { typescript: "^7" } },
},
{
// `bun install` does not install every peer, so `bun init` adds @types/bun beside a peer entry.
args: "-m -y",
where: "peerDependencies lists @types/bun",
groups: { peerDependencies: { "@types/bun": "1.1.0" } },
added: { devDependencies: { "@types/bun": "latest" } },
},
])("bun init $args adds only the dependency that is missing when $where", async ({ args, groups, added }) => {
const manifest = { ...configured, ...groups };

const after = await initOverPackageJson(manifest, args);

expect(after.manifest).toEqual({ ...manifest, ...added });
// `bun init` added a dependency, so the nested `bun install` runs and asks for @types/bun.
expect(after.requests).toContain("/@types/bun");
Comment on lines +439 to +441

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.

🟡 nit (optional): maintainers get a test matrix that never checks how bun init exited, unlike its sibling matrix at test/cli/init/init.test.ts:411. The second test.each at test/cli/init/init.test.ts:439-441 asserts only the manifest and the registry requests, while initOverPackageJson already returns exitCode, stdout and stderr. test/CLAUDE.md says to always check exit codes. Fix: assert exitCode is 0 for every row (for example expect(after).toMatchObject({ exitCode: 0 }) as the first matrix does), so a crash or error exit after the manifest write still fails the test.

Why this was flagged

The second parameterized test added at test/cli/init/init.test.ts:414-442 runs bun init over a prepared package.json via initOverPackageJson (test/cli/init/init.test.ts:352-381), which returns { manifest, requests, stdout, stderr, exitCode }. The test body at test/cli/init/init.test.ts:439-441 only checks after.manifest and after.requests; after.exitCode is never read. The sibling matrix in the same diff checks it at test/cli/init/init.test.ts:411. test/CLAUDE.md:201 reads "Always check exit codes and test error scenarios". The nested install failure is ignored by let _ = bun::spawn_sync_inherit(...) at src/runtime/cli/init_command.rs:943, so bun init exits 0 here today and the assertion is cheap; without it a future crash or nonzero exit that happens after package.json is written passes these three rows. The base branch has neither test, so this is a convention slip in the new code only.

Verification: nit. In the diff of test/cli/init/init.test.ts, the second matrix asserts only after.manifest and after.requests; after.exitCode (returned by initOverPackageJson) is never read. test/CLAUDE.md:201 reads "Always check exit codes and test error scenarios". A bun init that writes the manifest, triggers the install request, and then exits nonzero would still pass these three rows.

});

test("bun init --react works", async () => {
await using temp = tempDir("bun-init--react-works", {});

Expand Down
Loading