Conversation
|
Updated 6:09 PM PT - Jul 24th, 2026
✅ @robobun, your commit ea756e34a71ac8b762b3d21033bf2867cc08485b passed in 🧪 To try this PR locally: bunx bun-pr 35445That installs a local version of the PR into your bun-35445 --bun |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (6)
Comment |
There was a problem hiding this comment.
LGTM — mechanical move of typescript from peerDependencies to devDependencies across the init scaffolding.
What was reviewed:
init_command.rs: the two sequentialdevDependenciesget/put blocks compose correctly (second reads back the object the first wrote), andneeds_typescript_dependencystill checkspeerDependenciesso an existing user entry there is preserved rather than duplicated.- Template
package.json+bun.lockworkspace headers: only the root workspace entry moved; remainingpeerDependencieshits in the lockfiles are third-party package metadata, correctly untouched. docs/quickstart.mdxexample updated to match the new output.- Test assertions cover all four templates plus the "init twice" re-run path.
Extended reasoning...
Overview
This PR moves the typescript dependency that bun init writes into scaffolded projects from peerDependencies to devDependencies, fixing #7040. It touches: the Rust init command (init_command.rs, a 3-identifier rename in one block), the three embedded React template package.json files and their checked-in bun.lock workspace headers, the quickstart docs example, and the init test file's assertions.
Security risks
None. This only affects which JSON key a build-time dependency is written under in newly scaffolded projects. No untrusted input handling, no filesystem/path logic, no auth/crypto.
Level of scrutiny
Low. The Rust change is a straightforward rename (peerDependencies → devDependencies) in a block that already followed the exact get-or-create/mutate/put pattern used by the adjacent needs_dev_dependencies block. I verified the two blocks don't clobber each other: the first object.put(b"devDependencies", ...) stores the object with @types/bun, and the second object.get(b"devDependencies") retrieves that same stored object before adding typescript — the tests confirm both entries land in the output. The needs_typescript_dependency guard was intentionally left checking both devDependencies and peerDependencies, so re-running init on a project where the user deliberately kept typescript as a peer dep won't add a duplicate.
The template/lockfile changes are pure JSON key relocation. Grepping src/runtime/cli/init for peerDependencies after the change shows only third-party package metadata in the lockfile packages section, which is correct.
Other factors
Test coverage is thorough: exact-object assertions for the blank template, toHaveProperty("devDependencies.typescript", "^6") + peerDependencies === undefined for all three React templates, and the "bun init twice" snapshot. The pre-existing installs TypeScript 6, typechecks, and builds matrix test guards against install-time regressions from the lockfile header edits. The rationale (peer deps are semantically wrong for a build tool in an app scaffold; matches create-vite/create-next-app) is sound and uncontroversial.
There was a problem hiding this comment.
LGTM — mechanical move of typescript from peerDependencies to devDependencies across the init scaffolds.
What was reviewed:
init_command.rs: theneeds_typescript_dependencyblock now targetsdevDependencies; it runs after theneeds_dev_dependenciesblock, so the secondobject.get(b"devDependencies")sees@types/bunalready added and both entries land in the same object (confirmed by the updatedtoEqualsnapshot ininit.test.ts).- The skip-if-already-declared check still probes both
devDependenciesandpeerDependencies, so re-initing over a pre-existing peerDep doesn't duplicate. - Template
bun.lockedits touch only the workspace-root header; the resolvedpackagesmap is unchanged (typescript was already installed as an auto-installed peer), so the lockfiles stay consistent. - Grepped
src/runtime/cli/init/anddocs/for otherpeerDependencies+typescript references — none remain.
Extended reasoning...
Overview
This PR moves the typescript: "^6" entry that bun init writes from peerDependencies to devDependencies. It touches: a 3-identifier rename in src/runtime/cli/init_command.rs (the needs_typescript_dependency block), three include_bytes!'d template package.json files (react-app/tailwind/shadcn) plus their checked-in bun.lock workspace headers, the docs/quickstart.mdx example, and test/cli/init/init.test.ts assertions.
Security risks
None. This only changes which JSON key a scaffolding template writes a version string under. No user input parsing, no filesystem/path handling changes, no auth/crypto.
Level of scrutiny
Low. The Rust change is a pure identifier substitution reusing the exact same object.get(...).unwrap_or_else(...) / put_string / object.put pattern as the needs_dev_dependencies block immediately above it. I verified the ordering: needs_dev_dependencies runs first and puts the updated devDependencies back onto object, so the subsequent object.get(b"devDependencies") in the typescript block returns the object already containing @types/bun — the bun init works test's toEqual on the full package.json confirms both entries coexist. The lockfile edits only touch the workspaces[""] header (the resolved packages map is byte-identical, since typescript was already resolved as an auto-installed peer). The template package.json edits are trivial JSON section moves.
Other factors
- The pre-existing
needs_typescript_dependencyguard still checks bothdevDependenciesandpeerDependencies, so a user who deliberately keeps typescript under peerDependencies won't get a duplicate on re-init. - Test coverage is updated for every template (
-y,--react,--react=tailwind,--react=shadcn) to assertdevDependencies.typescript === "^6"andpeerDependencies === undefined, and the existinginstalls TypeScript 6, typechecks, and buildsmatrix test still covers thattsc --noEmitpasses with the installed TypeScript. - No CODEOWNERS match the changed paths.
- Grepped for any remaining
peerDependencies+typescript references insrc/runtime/cli/init/anddocs/— the only remainingpeerDependenciesoccurrences in the template lockfiles are the peer-dep declarations of resolved third-party packages (react-dom→react, etc.), which are unrelated.
There was a problem hiding this comment.
LGTM — mechanical move of typescript from peerDependencies to devDependencies across the init scaffold.
What was reviewed:
init_command.rs: the typescript block now targetsdevDependencies; ordering is fine since it re-gets after theneeds_dev_dependenciesblock'sput, so@types/bunis preserved (confirmed by the exact-equality assertion in "bun init works").- The retained
peerDependenciesskip-check means re-initing a project that already hastypescriptunder peerDeps won't duplicate it. - Grepped
src/runtime/cli/init/anddocs/for stalepeerDependencies+typescript references — none remain (leftoverpeerDependenciesin the bun.lock files are transitive package metadata, not the workspace root). - The one CI failure (
test-worker-message-port-transfer-terminate.jsSIGABRT on x64-asan) is unrelated to this change.
Extended reasoning...
Overview
Moves the typescript: "^6" entry that bun init writes from peerDependencies to devDependencies. Touches the Rust init command (3-line variable/key rename in the needs_typescript_dependency block), the three include_bytes!'d React template package.json files and their checked-in bun.lock workspace headers, the quickstart doc, and updates init.test.ts assertions to match.
Security risks
None. This is scaffolding metadata for newly-created projects; no auth, crypto, path handling, or untrusted input is involved.
Level of scrutiny
Low. The Rust change reuses the identical get-or-create/put_string/put pattern from the needs_dev_dependencies block immediately above it, just with a different key. The template edits are pure JSON key relocation. The behavior change is user-visible but is a straightforward correction to match ecosystem convention (create-vite, create-next-app, etc.) and fixes a filed issue.
Other factors
- Test coverage is thorough: exact-equality assertions on the blank template's package.json,
toHaveProperty("devDependencies.typescript", "^6")+peerDependenciestoBeUndefined for all three React templates, and the existinginstalls TypeScript 6, typechecks, and buildsmatrix test still exercises the install path. - Verified no sibling sites were missed: grepped for
peerDependenciesundersrc/runtime/cli/init/and for typescript+peerDependencies underdocs/— all remaining hits are transitive package metadata in the lockfiles'packagessections, not the workspace root. - The lone CI failure is a known-flaky worker termination ASAN test with no connection to
bun init.
Fixes #7040.
Repro
Before:
{ "name": "x", "module": "index.ts", "type": "module", "private": true, "devDependencies": { "@types/bun": "latest" }, "peerDependencies": { "typescript": "^6" } }After:
{ "name": "x", "module": "index.ts", "type": "module", "private": true, "devDependencies": { "@types/bun": "latest", "typescript": "^6" } }Why
peerDependenciesdeclares "my consumer must install this". For a scaffolded application that has no consumer, the field is meaningless. For a scaffolded library it is actively wrong: publishing the scaffold as-is makes every installer of the library warn (or, with npm's auto-install-peers, pull in)typescript@^6even in a plain-JS project. TypeScript is a build-time tool; it belongs indevDependencies, which is where every other scaffolding tool that adds it (create-vite,create-next-app,tsc --initworkflows, etc.) puts it.Fix
init_command.rs: writetypescriptintodevDependenciesinstead ofpeerDependencies. The existing "skip if already declared" check (devDependencies or peerDependencies) is kept, so a user who deliberately put it underpeerDependencieskeeps their version and we don't duplicate it.react-app/react-tailwind/react-shadcnpackage.json+ their checked-inbun.lockworkspace header): move the entry accordingly. These package.json files areinclude_bytes!'d directly, so they're the source of truth forbun init --react*.Verification
test/cli/init/init.test.tsis updated to assertdevDependencies.typescript === "^6"andpeerDependencies === undefinedfor every template. The existinginstalls TypeScript 6, typechecks, and buildsmatrix test confirms TypeScript is still installed andtsc --noEmitstill passes.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/init/init.test.ts