-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(rrweb-snapshot): align _cssText split points with css rule boundaries #1920
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "rrweb-snapshot": patch | ||
| --- | ||
|
|
||
| Fix `<style>` rules being dropped on replay when a mutation inserts a text node between two `_cssText` splits. The split points recorded by `markCssSplits` regularly land in the middle of a rule, which produces invalid css once a sibling is inserted between the parts; `applyCssSplits` now moves each split point forward to the end of the rule it lands in. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,25 @@ | ||
| { | ||
| "extends": "../../../tsconfig.base.json", | ||
| "include": ["src"], | ||
| "exclude": ["vite.config.ts", "test"], | ||
| "include": [ | ||
| "src" | ||
| ], | ||
| "exclude": [ | ||
| "vite.config.ts", | ||
| "test" | ||
| ], | ||
| "compilerOptions": { | ||
| "rootDir": "src", | ||
| "tsBuildInfoFile": "./tsconfig.tsbuildinfo" | ||
| }, | ||
| "references": [ | ||
| { | ||
| "path": "../rrweb-plugin-network-record" | ||
| }, | ||
| { | ||
| "path": "../../types" | ||
| }, | ||
| { | ||
| "path": "../rrweb-plugin-network-record" | ||
| "path": "../../rrweb" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,11 @@ import { describe, it, beforeEach, expect } from 'vitest'; | |
| import { mediaSelectorPlugin, pseudoClassPlugin } from '../src/css'; | ||
| import postcss, { type AcceptedPlugin } from 'postcss'; | ||
| import { JSDOM } from 'jsdom'; | ||
| import { splitCssText, stringifyStylesheet } from './../src/utils'; | ||
| import { | ||
| snapCssSplitsToRuleBoundaries, | ||
| splitCssText, | ||
| stringifyStylesheet, | ||
| } from './../src/utils'; | ||
| import { applyCssSplits } from './../src/rebuild'; | ||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
|
|
@@ -408,13 +412,74 @@ describe('applyCssSplits css rejoiner', function () { | |
| '/* rr_split */', | ||
| ); | ||
| applyCssSplits(sn3, markedCssText, true, mockLastUnusedArg); | ||
| // the split points move to the end of the rule they landed in, so each | ||
| // text node holds whole rules rather than half of one | ||
| expect((sn3.childNodes[0] as textNode).textContent).toEqual( | ||
| badStartThird.replace('.a:hover', '.a:hover,\n.a.\\:hover'), | ||
| '.a:hover,\n.a.\\:hover { background-color: red; }', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so IIRC the idea with this test was to test actual weird text nodes that don't land in logical css split points ... i.e. the original text nodes were injected arbitrarily ... it's a contrived example designed to show that we don't throw an error trying to parse any of the parts as valid CSS, so I'd reject this test modification, but rather ask you to find the root cause of what the styled-components was actually injecting (was there ever invalid css produced at record time ... if not we should not produce invalid css in our splitting ... that's the challenge) |
||
| ); | ||
| expect((sn3.childNodes[1] as textNode).textContent).toEqual( | ||
| badMidThird.replace('input:hover', 'input:hover,\ninput.\\:hover'), | ||
| ' input:hover,\ninput.\\:hover {border: 1px solid purple; }', | ||
| ); | ||
| expect((sn3.childNodes[2] as textNode).textContent).toEqual(''); | ||
| expect( | ||
| (sn3.childNodes[0] as textNode).textContent + | ||
| (sn3.childNodes[1] as textNode).textContent + | ||
| (sn3.childNodes[2] as textNode).textContent, | ||
| ).toEqual( | ||
| [badStartThird, badMidThird, badEndThird] | ||
| .join('') | ||
| .replace('.a:hover', '.a:hover,\n.a.\\:hover') | ||
| .replace('input:hover', 'input:hover,\ninput.\\:hover'), | ||
| ); | ||
| }); | ||
|
|
||
| it('moves a split point which lands inside a rule to the end of that rule', () => { | ||
| const markedCssText = [ | ||
| '.a { color: red; }.b { col', | ||
| 'or: green; }.c { color: blue; }', | ||
| ].join('/* rr_split */'); | ||
| applyCssSplits(sn, markedCssText, false, mockLastUnusedArg); | ||
| expect((sn.childNodes[0] as textNode).textContent).toEqual( | ||
| '.a { color: red; }.b { color: green; }', | ||
| ); | ||
| expect((sn.childNodes[1] as textNode).textContent).toEqual( | ||
| '.c { color: blue; }', | ||
| ); | ||
| }); | ||
|
|
||
| it('survives a sibling text node being inserted between the splits', () => { | ||
| // a later mutation can insert a text node between these two, which is the | ||
| // whole reason the split is preserved; the css has to stay valid when it | ||
| // does | ||
| const markedCssText = [ | ||
| '.a { color: red; }.b { col', | ||
| 'or: green; }.c { color: blue; }', | ||
| ].join('/* rr_split */'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is starting with a broken split if I understand it correctly — is this a simulation of the 'bad/inexact' splitting? |
||
| applyCssSplits(sn, markedCssText, false, mockLastUnusedArg); | ||
| const inserted = '.inserted { color: pink; }'; | ||
| expect( | ||
| (sn.childNodes[0] as textNode).textContent + | ||
| inserted + | ||
| (sn.childNodes[1] as textNode).textContent, | ||
| ).toEqual( | ||
| '.a { color: red; }.b { color: green; }' + | ||
| inserted + | ||
| '.c { color: blue; }', | ||
| ); | ||
| }); | ||
|
|
||
| it('does not mistake braces inside strings for the end of a rule', () => { | ||
| const markedCssText = [ | ||
| '.a::after { content: "}', | ||
| '"; }.b { color: red; }', | ||
| ].join('/* rr_split */'); | ||
| applyCssSplits(sn, markedCssText, false, mockLastUnusedArg); | ||
| expect((sn.childNodes[0] as textNode).textContent).toEqual( | ||
| '.a::after { content: "}"; }', | ||
| ); | ||
| expect((sn.childNodes[1] as textNode).textContent).toEqual( | ||
| '.b { color: red; }', | ||
| ); | ||
| expect((sn3.childNodes[2] as textNode).textContent).toEqual(badEndThird); | ||
| }); | ||
|
|
||
| it('maintains entire css text when there are too few child nodes', () => { | ||
|
|
@@ -434,3 +499,49 @@ describe('applyCssSplits css rejoiner', function () { | |
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('snapCssSplitsToRuleBoundaries', function () { | ||
| it('leaves splits which already sit on a rule boundary alone', () => { | ||
| const splits = ['.a { color: red; }', '.b { color: green; }']; | ||
| expect(snapCssSplitsToRuleBoundaries(splits)).toEqual(splits); | ||
| }); | ||
|
|
||
| it('moves a split point forward to the end of the rule', () => { | ||
| expect( | ||
| snapCssSplitsToRuleBoundaries([ | ||
| '.a { col', | ||
| 'or: red; }.b { color: green; }', | ||
| ]), | ||
| ).toEqual(['.a { color: red; }', '.b { color: green; }']); | ||
| }); | ||
|
|
||
| it('ignores braces inside strings and comments', () => { | ||
| expect( | ||
| snapCssSplitsToRuleBoundaries([ | ||
| '.a { content: "}"; /* } */ ', | ||
| '}.b { color: red; }', | ||
| ]), | ||
| ).toEqual(['.a { content: "}"; /* } */ }', '.b { color: red; }']); | ||
| }); | ||
|
|
||
| it('keeps nested at-rules together', () => { | ||
| expect( | ||
| snapCssSplitsToRuleBoundaries([ | ||
| '@media print { .a { color', | ||
| ': red; } }.b { color: green; }', | ||
| ]), | ||
| ).toEqual(['@media print { .a { color: red; } }', '.b { color: green; }']); | ||
| }); | ||
|
|
||
| it('empties trailing splits when everything snapped into an earlier one', () => { | ||
| expect( | ||
| snapCssSplitsToRuleBoundaries(['.a { col', 'or', ': red; }']), | ||
| ).toEqual(['.a { color: red; }', '', '']); | ||
| }); | ||
|
|
||
| it('is a no-op for a single split', () => { | ||
| expect(snapCssSplitsToRuleBoundaries(['.a { color'])).toEqual([ | ||
| '.a { color', | ||
| ]); | ||
| }); | ||
| }); | ||
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'm assuming all the bad cases are produced in here ("something went wrong") so possibly we could confine the nextCssRuleBoundary call to only in here.
I'd also be interested in the input data that caused the above search to go wrong.
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.
Above comment was written before I realized we are in rebuild here not snapshot ...