Repository navigation
install --analyze: read the import records before the link #46602
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
Open
robobun
wants to merge
3
commits into
main
Choose a base branch
from
robobun/658b944b/analyze-reads-imports-before-link
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔴 Users running
bun install --analyzeon a project whose default-imported HTML page has a bare asset URL now get a bogus npm package installed instead of an error. The new early return at bundle_v2.rs:4518 skips the link that used to stop such pages, so<img src="images/photo.webp">reachesget_all_dependenciesas the external importimages/photo.webpandimagesis added to package.json and fetched from the registry (pinned by bun-add.test.ts:527). Fix: a bare URL in an HTML page or a CSSurl()must never become a package name on the --analyze path, e.g. drop theext.len() > 4cut at HTMLScanner.rs:88 or skip records whose importer is an HTML/CSS file inget_all_dependencies. [also at: test/cli/install/bun-add.test.ts:527 - Users of abun init --reactproject whose page has<img src="images/photo.webp">now get an npm package named…]Why this was flagged
A user has
import index from "./index.html"in src/index.ts (what everybun init --react*template generates) and adds<img src="images/photo.webp">to the page, then runsbun install --analyze src/index.ts. On the base branchgenerate_from_clilinks first and exits 1 withNo matching export in "src/index.html" for import "default", so nothing is installed. After this change bundle_v2.rs:4515-4518 returnsget_all_dependenciesright afterfind_reachable_files, beforelinker.link.HTMLScanner::create_import_recordat src/bundler/HTMLScanner.rs:81-102 only tries the relative path when the extension is at most 4 bytes, so.webpstays bare; withpackages = externalthe resolver marks it external andget_all_dependenciesat bundle_v2.rs:4432 parsesimagesas the package name and inserts it. The PR's own test at test/cli/install/bun-add.test.ts:522-527 assertsadded: ["bar", "images"]. No safeguard remains on this path: the registry 404 check only fires if the name does not exist on npm, andimagesdoes.Verification: bundle_v2.rs:4515-4518 returns
get_all_dependencieswhenfetch.reads == ImportRecords, skippinglinker.link(4545-4547). HTMLScanner.rs:88 tries a bare path as a relative file only whenext.len() <= 4, soimages/photo.webpstays verbatim; resolver.rs:976-979 marks it external and bundle_v2.rs:4426-4442 insertsimages. On base, exit 1 and nothing installed; after this change, the unrelated npm packageimagesis installed.An automated safety screen replaced some characters, such as angle brackets, in the code quoted in this comment with HTML escape codes.
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.
Confirmed, and it is stated under Downsides. Two facts change where the fix has to live:
imagestoday without this PR. Measured on main 5c6d4e2:import * as page from "./page.html"with this<img>,bun install --analyze page.html, and a plainimport "./styles.css"whose stylesheet hasurl(images/bg.png)each addimages. This PR adds the default import as one more way in.HTMLScanneronly matters for HTML, so CSSurl()stays wrong. A filter on HTML or CSS importers inget_all_dependenciesdrops real packages: the scan findstailwindcssfor thebun init --react=tailwindtemplate through@import "tailwindcss"in its CSS.The fault is how URL records resolve under
packages = external: a bare path is external before any file beside the importer is tried. That fix is separate work and is in progress. 119ae67 turns the case into anit.failingthat asserts["bar"], so the fix removes.failing. The description now says that the fix should land before or with this PR. I leave this thread open for a maintainer to decide the order.