-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: nested stylesheets should have absolute URLs #1533
fix: nested stylesheets should have absolute URLs #1533
Conversation
🦋 Changeset detectedLatest commit: acf1cb9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
7c329f9
to
8f6498d
Compare
88fbb74
to
c6c0ee3
Compare
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.
I think we just need to avoid the extra .map
call as per my other comment; I can make the other changes at a later date.
…ter stringifying imported stylesheet
f26eacf
to
27432e2
Compare
packages/rrweb-snapshot/test/__snapshots__/integration.test.ts.snap
Outdated
Show resolved
Hide resolved
So something in 27432e2 has caused the test to fail; unfortunately they didn't run automatically after you pushed to the PR, not sure why |
I've also got 3 commits in https://github.com/eoghanmurray/rrweb/tree/jn-correct-absolute-urls-for-nested-stylesheets that I'd like you to cherry-pick / pull in here first ... one of them adds a new test. |
Good catch! I think I know why, moving the |
Weird...I'm not seeing that checkbox but will look into that. In the meantime happy to cherry pick! |
Also I'm urging the rest of the core team to merge in the following soon: I imagine there would be extensive conflicts with this one, so if you are at all going back to the drawing board, then we could keep the tests from this PR and build upon that first. If that's too much let's go ahead with this one and I'll handle the rebase on the other. |
Update
|
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.
Just the moving of code out of the try/catch and I think we'll be good to merge
Made the change! 🤞 |
I've made a few more changes on top, including playing around with a benchmark as I was worried about the now-per-rule calls to |
Cherry-picked your commits in. Nice job with deduping the fallback behavior, that feels much better! Glad the benchmark turned out fine, did not think to do that 😅 |
@eoghanmurray Please let me know if there is anything left I need to do to or can help with to get this merged 😄 |
Can we use postcss instead of regex to replace/absolutify urls in the css? We've had a lot of issues with regular expressions not being the same as the spec and having some buggy edge cases. We've added postcss recently which should be much more stable. |
@Juice10 I love the idea! Would you be open to me opening a seperate/followup PR to explore that instead? Mainly since I wanted to keep the scope of this PR to changing when we try to |
@Juice10 this PR is for the record side; postcss is used on the replay side, so big -1 from me for introducing postcss dependency for recording. We've been using the absoluteURLs regex for ages already (this doesn't change that) and nobody has reported any issues. I did explore a couple of further commits on top of this PR which isolate the url regex replace to just the rule bit of the css (and not the selector bit). I had performance rather than correctness in mind with that, as after this PR we are applying the regex to each rule, rather than to the entire block of css. I figured this PR was complete enough so will introduce those changes properly in a new PR if this is accepted. Side note: postcss could be made redundant at replay time if I complete #1480 successfully. |
Fair point @eoghanmurray! Merging this as is, thanks for contributing @jeffdnguyen |
The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in rrweb-io#1533 but are caught in this PR by the tests added in that PR as the stylesheet handling is simplified/centralised
The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in rrweb-io#1533 but are caught in this PR by the tests added in that PR as the stylesheet handling is simplified/centralised
* Replace relative URLs with absolute URLs when stringifying stylesheets * Add test to show desired behavior for imported stylesheets from seperate directory * Rename `absoluteToStylesheet` to `absolutifyURLs` and call it once after stringifying imported stylesheet * Don't create the intermediary array of the spread operator * Formalize that `stringifyRule` should expect a sheet href * Ensure a <style> element can also import and gets it's url absolutized * Handle case where non imported stylesheet has relative urls that need to be absolutified * Clarify in test files where jpegs are expected to appear in absolutified urls * Move absolutifyURLs call for import rules out of trycatch * Add a benchmarking test for stringifyStylesheet * Avoid the duplication on how to fall back --------- Co-authored-by: Eoghan Murray <[email protected]> Co-authored-by: eoghanmurray <[email protected]>
* Replace relative URLs with absolute URLs when stringifying stylesheets * Add test to show desired behavior for imported stylesheets from seperate directory * Rename `absoluteToStylesheet` to `absolutifyURLs` and call it once after stringifying imported stylesheet * Don't create the intermediary array of the spread operator * Formalize that `stringifyRule` should expect a sheet href * Ensure a <style> element can also import and gets it's url absolutized * Handle case where non imported stylesheet has relative urls that need to be absolutified * Clarify in test files where jpegs are expected to appear in absolutified urls * Move absolutifyURLs call for import rules out of trycatch * Add a benchmarking test for stringifyStylesheet * Avoid the duplication on how to fall back --------- Co-authored-by: Eoghan Murray <[email protected]> Co-authored-by: eoghanmurray <[email protected]>
The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in rrweb-io#1533 but are caught in this PR by the tests added in that PR as the stylesheet handling is simplified/centralised
Support a contrived/rare case where a <style> element has multiple text node children (this is usually only possible to recreate via javascript append) ... this PR fixes cases where there are subsequent text mutations to these nodes; previously these would have been lost * In this scenario, a new CSS comment may now be inserted into the captured `_cssText` for a <style> element to show where it should be broken up into text elements upon replay: `/* rr_split */` * The new 'can record and replay style mutations' test is the principal way to the problematic scenarios, and is a detailed 'catch-all' test with many checks to cover most of the ways things can fail * There are new tests for splitting/rebuilding the css using the rr_split marker * The prior 'dynamic stylesheet' route is now the main route for serializing a stylesheet; dynamic stylesheet were missed out in #1533 but that case is now covered with this PR This PR was originally extracted from #1475 so the initial motivation was to change the approach on stringifying <style> elements to do so in a single place. This is also the motivating factor for always serializing <style> elements via the `_cssText` attribute rather than in it's childNodes; in #1475 we will be delaying populating `_cssText` for performance and instead recorrding them as assets. Thanks for the detailed review to Justin Halsall <[email protected]> & Yun Feng <https://github.com/YunFeng0817>
* Replace relative URLs with absolute URLs when stringifying stylesheets * Add test to show desired behavior for imported stylesheets from seperate directory * Rename `absoluteToStylesheet` to `absolutifyURLs` and call it once after stringifying imported stylesheet * Don't create the intermediary array of the spread operator * Formalize that `stringifyRule` should expect a sheet href * Ensure a <style> element can also import and gets it's url absolutized * Handle case where non imported stylesheet has relative urls that need to be absolutified * Clarify in test files where jpegs are expected to appear in absolutified urls * Move absolutifyURLs call for import rules out of trycatch * Add a benchmarking test for stringifyStylesheet * Avoid the duplication on how to fall back --------- Co-authored-by: Eoghan Murray <[email protected]> Co-authored-by: eoghanmurray <[email protected]>
hello 👋 - I identified an issue with nested stylesheets while looking into the asset manager PR.
The PR has a comment that outlines the problem in more detail
The tl;dr is that for style tags, we are assuming any URLs inside of a style tag must share the same file path as the document the style tag is on. This causes nested stylesheets (think style tag that imports another stylesheet) to have incorrect urls in the snapshot. In order to fix this, I decided the best route would be to determine those absolute paths sooner when stringifying the stylesheets.