-
Notifications
You must be signed in to change notification settings - Fork 4.9k
ShadowRealm: cache importValue module in its own realm under require(esm) #36321
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
2
commits into
main
Choose a base branch
from
farm/d889cf53/sync-queue-realm
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.
+43
−1
Open
Changes from all commits
Commits
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
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.
🔴
WEBKIT_VERSIONis pinned toautobuild-preview-pr-371-4e9f0037, an ephemeral preview tag that GitHub deletes when oven-sh/WebKit#371 merges or closes — at which point every fresh checkout/CI build 404s on the WebKit prebuilt download. Before merge, swap this to the merged main-branch SHA of oven-sh/WebKit (once #371 lands) and verify prebuilt artifacts exist for every platform × flavor.Extended reasoning...
What the bug is
scripts/build/deps/webkit.ts:6setsWEBKIT_VERSION = "autobuild-preview-pr-371-4e9f0037". This is anautobuild-preview-pr-*tag — a preview release published for an unmerged WebKit PR (oven-sh/WebKit#371), not a stable main-branch commit. The PR description itself acknowledges this: "This PR bumpsWEBKIT_VERSIONto that PR's preview build."Why this is merge-blocking
The repo's own review rules in
.claude/docs/landing-prs.md§ Dependencies & vendoring state explicitly:And the build system's own error handling confirms exactly what happens when this class of pin goes stale —
scripts/build/download.ts:278-294:Concrete failure walkthrough
mainwithWEBKIT_VERSION = "autobuild-preview-pr-371-4e9f0037".autobuild-preview-pr-371-4e9f0037release.bun bd.prebuiltUrl()inwebkit.tscomputeshttps://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-371-4e9f0037/bun-webkit-<os>-<arch><suffix>.tar.gz.prebuiltDownloadError()throws"WebKit preview release is gone"and the build fails.mainis now broken until someone lands a follow-up commit changingWEBKIT_VERSION.Nothing in the existing code prevents this — the dedicated error handler in
download.tsexists precisely because this failure mode has bitten before; it makes the crash legible but does not avert it.Impact
mainbecomes unbuildable from a clean cache the moment the upstream WebKit PR's lifecycle changes — a window that is entirely outside this repo's control.Fix
Before merging this PR:
main.WEBKIT_VERSIONto the resulting 40-hex main-branch commit SHA (the previous value549170099226f816a4b204ea1d8fa102fb79eefais stated to be the parent, so only Fix typo #371's change rides along).https://github.com/oven-sh/WebKit/releases/tag/autobuild-<sha>for every platform × {debug, lto, asan, musl} flavor the build matrix consumes.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.
Correct, this is the staging state so CI can exercise the fix. Before merge this gets swapped to the 40-hex main SHA once oven-sh/WebKit#371 lands and its
autobuild-<sha>release is published (the preview build parent is the currently pinned549170099, so only that one commit rides along).