docs(bundler): clarify that publicPath does not rewrite external module specifiers - #35577
docs(bundler): clarify that publicPath does not rewrite external module specifiers#35577robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 10 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 selected for processing (5)
Comment |
|
Updated 6:06 PM PT - Jul 25th, 2026
❌ @robobun, your commit 2c89c08 has some failures in 🧪 To try this PR locally: bunx bun-pr 35577That installs a local version of the PR into your bun-35577 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
LGTM — thanks for dropping the onResolve sentence; the remaining docs-only change is accurate.
Checked: the three publicPath bullets (assets, chunk imports, linked sourcemap URL) match where the bundler actually applies the prefix; the removed "External modules" bullet was the misleading one #11652 reported; the #external anchor resolves to the existing ### external section. No code touched.
One leftover: the PR description still says Fixes #11652 and describes the reverted code fix/tests — worth trimming so the auto-close doesn't surprise anyone, but not blocking.
Extended reasoning...
Overview
After commits 844295c and 9d9bafc, this PR is a pure documentation change to docs/bundler/index.mdx. It rewords the publicPath section to list the three cases where the prefix is actually applied (file-loader asset paths, cross-chunk imports under --splitting, and the //# sourceMappingURL= comment for sourcemap: "linked"), removes the misleading "External modules" bullet, and adds an explicit sentence that external specifiers are emitted unchanged. The earlier revisions of this PR contained a code fix in src/bundler/bundle_v2.rs and bundler tests; those were reverted in favor of the pre-existing #35053, and my prior feedback about the dangling onResolve-workaround sentence was addressed in 9d9bafc.
Security risks
None. This is a prose-only change to an .mdx documentation file with no executable content.
Level of scrutiny
Low. Docs-only, ~10 lines of prose diff. The claims are verifiable against existing behavior: the sourcemap table in the same file already documents that --public-path customizes the linked sourcemap URL, and the external section already states externals are "left as-is, to be resolved at runtime" — this change just cross-references that from the publicPath section where users hit the confusion. esbuild parity (mentioned in the PR description) matches. No CODEOWNERS entry covers docs/.
Other factors
Both of my earlier inline findings on this PR concerned code that has since been removed from the diff (the bundle_v2.rs deferred-rewrite ordering issue, and the docs sentence recommending an onResolve pattern that depends on unlanded #35053). Both threads are resolved and no longer apply to the current revision. CI passed on 844295c and 9d9bafc only deletes one sentence relative to that. The PR description is stale (still describes the code fix and tests, and carries Fixes #11652), but that doesn't affect the correctness of what's being merged.
There was a problem hiding this comment.
LGTM — the onResolve sentence that depended on #35053 has been dropped (9d9bafc), and the earlier bundle_v2.rs concern is moot now that the code fix was removed in favour of #35053. What remains is a docs clarification, a help-string reword, and a test pinning the help text.
Checked: the new publicPath bullets (assets, chunks, linked source maps) match what the bundler actually prefixes and are consistent with the existing sourcemap: "linked" docs; the added paragraph no longer recommends any mechanism that isn't shipped; the --help test drains stdout/stderr concurrently and asserts exit code last.
Extended reasoning...
Overview
Four files: docs/bundler/index.mdx rewrites the publicPath section to list the three cases it actually affects (file-loader asset paths, cross-chunk imports under --splitting, and the sourceMappingURL comment for linked source maps) and adds an explicit note that external specifiers are emitted unchanged; src/runtime/cli/Arguments.rs and docs/snippets/cli/build.mdx reword the --public-path description to match; test/bundler/cli.test.ts adds a test asserting the help line names asset/chunk/source-map.
An earlier revision carried a code fix in bundle_v2.rs (preserving the plugin-returned path when external: true). That was dropped in 844295c in favour of #35053, which mooted my earlier 🟡 finding about the deferred-rewrite ordering. My 🔴 finding — that the docs then recommended an onResolve workaround that doesn't work without #35053 — was addressed in 9d9bafc by removing that sentence. The current publicPath paragraph reads only: "publicPath does not rewrite specifiers for external modules. An import marked as external is emitted unchanged so it can be resolved at runtime." — accurate and self-contained.
Security risks
None. Documentation prose, a static help string, and a subprocess-based test that runs bun build --help.
Level of scrutiny
Low. The only compiled change is a string literal in parse_param!. The docs claims were verified against the existing sourcemap section (which already says --public-path customises the linked-map base URL) and the PR's own explanation of esbuild parity. The new test follows harness conventions (await using, bunEnv, concurrent pipe drain, exit-code asserted last).
Other factors
All prior inline threads on this PR are resolved and correspond to code that no longer exists in the diff or feedback that was applied. CI passed on 9d9bafc; c93a39c is a wording-alignment follow-up touching only the same help string and CLI snippet. The PR description still auto-closes #11652 — that's a metadata judgement (the reporter accepted a docs resolution) and doesn't affect the code.
There was a problem hiding this comment.
LGTM — the earlier concerns are addressed: the bundle_v2.rs change was dropped (so the deferred-rewrite ordering issue no longer applies here), and 9d9bafc removed the onResolve workaround sentence that depended on unlanded #35053. What remains is a docs clarification plus a one-word help-string change and a test pinning it. The inline nit about completions/bun-cli.json is worth doing but doesn't block.
Extended reasoning...
Overview
The current revision touches four files: docs/bundler/index.mdx (rewrites the publicPath section to enumerate the three cases it actually affects — asset paths, chunk imports, linked source maps — and adds a sentence stating externals are emitted unchanged), docs/snippets/cli/build.mdx and src/runtime/cli/Arguments.rs (align the --public-path help line with the docs), and test/bundler/cli.test.ts (asserts the help line names asset/chunk/source-map). Earlier revisions carried a bundle_v2.rs code fix and an onResolve docs recommendation; both were dropped in 844295c and 9d9bafc respectively in response to prior review comments, and those threads are resolved.
Security risks
None. This is documentation prose plus a static help-text literal inside a parse_param! macro. No parsing, no user input handling, no runtime behavior changes.
Level of scrutiny
Low. The Rust change is a string literal edit with no effect on argument parsing (only the description column of --help output changes). The docs edits are factually accurate — publicPath is applied to file-loader asset URLs, cross-chunk import paths under --splitting, and the sourceMappingURL comment; external specifiers are left as-is by design (esbuild parity). The new test follows harness conventions (await using, bunEnv, concurrent pipe drain, exit-code asserted last).
Other factors
- Both of my earlier findings targeted code that is no longer in this PR; the author addressed them by removing the offending code/sentence rather than defending it, which is the right call given #35053 owns the code fix.
- The one remaining nit (stale
--public-pathdescription in the checked-incompletions/bun-cli.json) is real but has no user-visible effect today — the shipped shell completion scripts don't carry that string, and the JSON will pick up the new text the next time the generator is run. Not a merge blocker. - The
Fixes #11652in the description is defensible as a docs-only resolution since the reporter said a clarification would suffice; the actual behavioral change is tracked separately in #35053.
f8f3008 to
2c89c08
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed (onResolve sentence dropped in 9d9bafc, completions JSON synced in 2c89c08, code fix deferred to #35053).
What was reviewed:
- Verified the new
publicPathdocs accurately describe the three affected cases (assets, chunks, linked source maps) and no longer reference the not-yet-working onResolve workaround. - Confirmed the old "appended to any import paths" wording is gone repo-wide;
Arguments.rs,build.mdx, andbun-cli.jsonnow agree. - Checked the new
cli.test.tshelp-text assertion — follows harness conventions (drains pipes concurrently, exit code asserted last).
Extended reasoning...
Overview
This PR clarifies that publicPath does not rewrite external module specifiers. It touches five files: docs/bundler/index.mdx (rewrites the publicPath section to enumerate the three cases it actually affects — asset imports, chunk imports under --splitting, and linked source-map URLs — and adds an explicit note that external specifiers are emitted unchanged), src/runtime/cli/Arguments.rs (one-line help-text string change for --public-path), docs/snippets/cli/build.mdx (matching CLI reference wording), completions/bun-cli.json (syncs the generated completions description), and test/bundler/cli.test.ts (a new test asserting the --public-path line in bun build --help mentions asset/chunk/source-map).
Earlier revisions of this PR carried a code fix in bundle_v2.rs for the onResolve { path, external: true } path; that was dropped in 844295c in favour of #35053, and my follow-up feedback about the docs then recommending a not-yet-working mechanism was addressed in 9d9bafc by removing that sentence. The stale completions/bun-cli.json entry I flagged was synced in 2c89c08. All three prior inline comments are resolved and reflected in the current diff.
Security risks
None. This is documentation prose plus a static help-text string. The only executable change is a parse_param! string literal in Arguments.rs that is displayed verbatim in --help output; it does not affect argument parsing, resolution, or any bundler behaviour.
Level of scrutiny
Low. The Rust change is a display string only; the docs changes describe existing behaviour more precisely (and match esbuild's semantics, which the bundler comparison page already claims parity with). The completions JSON is a checked-in artifact derived from the same help string. The new test is additive, hermetic (spawns bun build --help with bunEnv, no filesystem or network), and follows the harness conventions in CLAUDE.md — pipes drained via Promise.all, exit code asserted last, issue URL comment.
Other factors
I grepped for the old wording ("appended to any import paths") repo-wide and found no remaining occurrences, so all copies are now consistent. The sourcemap docs table already stated that --public-path customises the sourceMappingURL base, so the new "Linked source maps" bullet is consistent with existing documentation. No CODEOWNERS cover the touched paths. The PR description still contains Fixes #11652; since the reporter explicitly said a docs clarification would be acceptable, closing that issue with this change is reasonable.
|
CI on this PR is blocked by a Buildkite agent outage, not by the diff: builds #80817 and #81176 both show every Locally the new This needs a rebuild once agents are back; no code changes pending. |
1 participant
Fixes #11652
What was reported
With
bun build --external react --public-path https://cdn.example.com/, the emitted bundle still saysimport react from "react". ThepublicPathdocs listed external modules alongside assets and chunks as cases where the bundle "may contain paths to other files", and bothbun build --helpand the CLI reference described it as "a prefix to be appended to any import paths", which together read as if--public-pathshould rewrite external specifiers.Why this is working as intended
publicPathprefixes paths the bundler itself writes into the bundle: file-loader asset paths, cross-chunkimportstatements under--splitting, and thesourceMappingURLcomment for linked source maps. Anexternalspecifier is, by definition, a string the bundler leaves alone so the runtime can resolve it; rewriting it would change which module the runtime loads. esbuild behaves the same way (esbuild --bundle --external:react --public-path=... --format=esmemitsimport react from "react"), and the bundler comparison page already documentspublicPathas having no differences from esbuild. The issue reporter noted a docs clarification would be an acceptable resolution.Change
docs/bundler/index.mdx: thepublicPathsection now lists the three cases it actually affects and states that external specifiers are emitted unchanged.src/runtime/cli/Arguments.rs,docs/snippets/cli/build.mdx,completions/bun-cli.json: the--public-pathdescription now reads "A prefix for emitted asset, chunk, and source-map paths" (also fixes "appended" for a prefix).test/bundler/cli.test.ts: asserts the--public-pathline inbun build --helpnames asset/chunk/source-map so the description stays accurate.The mechanism for rewriting an external specifier (an
onResolveplugin that returns{ path, external: true }) is being fixed separately in #35053. An earlier revision of this PR carried an overlapping code fix for that path, since dropped in favour of #35053.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/cli.test.ts