Skip to content
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

Don't serialize all cssRules if multiple text nodes exists #866

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

Juice10
Copy link
Contributor

@Juice10 Juice10 commented Mar 29, 2022

Emotion adds css rules by appending text child nodes to a style element. This currently causes all cssRules to get serialized each time an extra rule gets added. That leads to huge stylesheets in replay (45mb+ in some cases) and can freeze the browser whenever more rules get added.

This PR only serializes all .cssRules when one text node exists in a style element.


it('should serialize individual text nodes on stylesheets with multiple child nodes', () => {
const styleEl = render(`<style>body { color: red; }</style>`);
styleEl.append(document.createTextNode('section { color: blue; }'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a rule is also inserted here, would it be recorded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any rule added before the append would be wiped by the webbrowser. Any rule added after the append will not be picked up in rrweb-snapshot unfortunately. And I don't know of a good way of capturing those.
In rrweb these rule additions are captured by the observers.

This PR hopes that you either add css via the .sheet.inserRule api or .append(TEXT_NODE) api. It's not foolproof but it's better than what we had.

@Juice10
Copy link
Contributor Author

Juice10 commented Mar 31, 2022

I mentioned that emotion.sh is what cases this. But I’ve seen the exact same behavior happen with vue.js too

@Yuyz0112 Yuyz0112 merged commit 9ed6767 into master Mar 31, 2022
@Juice10 Juice10 deleted the serialize-style-text-node branch April 1, 2022 11:07
@eoghanmurray
Copy link
Contributor

@Juice10 Would a better solution be to run the stringifyStylesheet during serializeElementNode on <style> rather than serializeTextNode on it's children?

@Juice10
Copy link
Contributor Author

Juice10 commented Apr 4, 2024

@eoghanmurray if we can grab the unmodified text content from the stylesheet than that is preferred. stringifyStylesheet goes through styleEl.sheet.cssRules which reads a manipulated version of the stylesheet with only what the browser understands to be the styles. (leads to the following being skipped: unsupported browser features, browser hacks etc.)

@eoghanmurray
Copy link
Contributor

#1437 has some changes related to this.
I hadn't realized when I posted here that rrweb mutations call directly in to serializeTextNode when a text node mutation happens.
I'll be writing more tests for these scenarios in #1437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants