Capture stylesheets designated as rel="preload"
#1374
Conversation
🦋 Changeset detectedLatest commit: 96ceef2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
andrewpomeroy
force-pushed
the
capture-preload-stylesheets
branch
from
bafa526
to
effdb63
Compare
andrewpomeroy
force-pushed
the
capture-preload-stylesheets
branch
from
effdb63
to
83c7af7
Compare
There was a problem hiding this comment.
Thanks for creating this PR @andrewpomeroy! This is a nice addition to rrweb. I found two minor things, check them out and let me know what you think and then we can merge this.
|
Thanks @Juice10! Your suggestions make perfect sense. I went ahead and made those changes. |
|
Hi @Juice10 ! It looks like this is now failing tests, which look to be the same tests currently failing on master. I tried checking out #1366 and fixing them myself, but didn't get much traction. My team is looking to close out this issue on our end; no pressure at all intended, but what are the chances of this getting merged/released in the next few weeks? We just need to be able to make the call whether to fork or not. Thanks again for your contributions 🙂 |
|
@Juice10 Whoops, just saw your comment here https://rrweb.slack.com/archives/C01BYDC5C93/p1702300072935979?thread_ts=1701887171.474259&cid=C01BYDC5C93 ! I'm assuming it won't be much longer then 😄 Looking forward to it! |
|
@andrewpomeroy hang tight Andrew, unfortunately I wasn’t able to fix master in #1366 (I should close that one), but indeed as mentioned in slack I’m hoping to merge #1239 soon which does fix master, and then we can merge this PR and do a release. |
|
Great to hear! Thanks for the update. |
|
@andrewpomeroy hey Andrew, could you pull from master, ci is passing now |
* feat(Snapshot): Capture stylesheets designated as `rel="preload"` * fix(Snapshot): Harden asset file extension matching * Add changeset * chore: Lint * Tweak regex, add try-catch block on URL constructor
Shouldn't this get picked up as a mutation? And I think the mutation should then serialize the stylesheet correctly? |
It's been a while since I've looked at this, but to the best of my memory— these mutations don't trigger a full snapshot, which is when the stylesheet-inlining process would take place. |
…red twice after a mutation to rel="stylesheet" from rel="preload"
…red twice after a mutation to rel="stylesheet" from rel="preload"
|
Yes, thanks — I actually have since written some tests and done some more work on this; the problem was that the mutation weren't doing anything when an attribute change caused a to 'become a stylesheet' |
…red twice after a mutation to rel="stylesheet" from rel="preload"
|
Gotcha. Good thinking! I'm all for it. |
Encountered an issue where stylesheets failed to capture when they were referenced by link tag with
rel="preload"instead ofrel="stylesheet". This change will broaden the rules so that these preloaded stylesheets can also be recognized and captured at the snapshot phase.For background: This attribute assignment is commonly used as a performance hack for the initial loading phase of a page—meant to allow stylesheets to be preloaded—and is usually combined with
onload="this.rel = 'stylesheet'"so that it is still, upon load, recognized as a stylesheet at runtime. The snapshot phase, naturally, will miss these unless we tweak the link-tag pattern matching to look for it specifically. More info: https://stackoverflow.com/a/61889123In addition, I added reinforced pattern matching for both
.cssfile paths—for the aforementioned scenario and the existing.js/rel="prefetch"scenario—so that paths will still be matched if they include search parameters or fragments.