Keep a rewritten <script>'s original src for wombat - #337
Open
epheterson wants to merge 1 commit into
Open
epheterson wants to merge 1 commit into
epheterson wants to merge 1 commit into
Conversation
Bundlers identify a chunk by the raw getAttribute("src") rather than the
.src property, and they expect the string the server sent. Rewriting an
absolute src to a relative one therefore breaks them silently: Next.js
with Turbopack strips a leading "/_next/" to derive a chunk's name, the
strip stops matching once the leading slash is gone, every chunk
registers under a name nothing is waiting for, and the page never
hydrates. Vite is reported to be affected the same way.
wombat cannot repair this on its own. Its getAttribute override
un-rewrites URLs that wombat rewrote, and this one was rewritten here,
at scrape time, so extractOriginalURL finds no prefix it knows and hands
the value straight back. What it does have is __wb_orig_src, which it
checks first for script elements (retrieveWBOSRC) and which only the
rewriter can fill in.
Nothing is added when the tag is not a script, when it has no src, when
rewriting left the src alone, or when the attribute is already there —
that last one being HTML rewritten twice, where the attribute already
present holds the true original and a second would both duplicate it and
win, since a browser takes the first.
Measured on a warc2zim capture of draculatheme.com: the replayed page is
900px before and 19851px after, against 19858px live.
Fixes openzim/warc2zim#473
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 42 42
Lines 2677 2694 +17
Branches 393 398 +5
=========================================
+ Hits 2677 2694 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
|
I realize now that we actually have a similar kind of issues for other Could it be an opportunity to kill two birds with one stone? I appreciate the cautious idea of narrowing the change to the very strict minimum set to make what you know work, but at the same time we know that all this rewriting is still a bit in its infancy and would benefit from such improvements "at large". |
Contributor
Author
|
Sure I'm down either way. Main motivation is fixing our bug upstream, but happy to fix a wider scope. You can expand on this PR or I'll take another look and put a new change up later. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Bundlers identify a chunk by the raw
getAttribute("src")rather than the.srcproperty, and they expect the string the server sent. Rewriting an absolute src to a relative one breaks them silently: Next.js with Turbopack doest.startsWith("/_next/") ? t.slice(7) : tto derive a chunk name, the strip stops matching once the leading slash is gone, every chunk registers under a name nothing is waiting for, and the page never hydrates. Vite is reported to be affected the same way.wombat cannot repair this on its own. Its
getAttributeoverride un-rewrites URLs that wombat rewrote, and this one was rewritten here, at scrape time, soextractOriginalURLfinds no prefix it recognises and hands the value straight back. What it does have is__wb_orig_src, which it already checks first for script elements (retrieveWBOSRC) and which only the rewriter can fill in.So this fills it in. Nothing is added when the tag is not a script, when it has no src, when rewriting left the src alone, or when the attribute is already present — that last case being HTML rewritten twice, where the attribute already there holds the true original and a second one would both duplicate it and win, since a browser takes the first.
Measured on a warc2zim capture of draculatheme.com: the replayed page is 900px before and 19851px after, against 19858px live.
One judgement call worth your eyes: the attribute is added whenever rewriting changed the src, which includes relative-to-relative (
foo.js->../foo.js). That is the honest reading of "return what the server sent", but narrowing it to srcs that were root-absolute would cut the noise. Happy to change it.Fixes openzim/warc2zim#473