rum-recorder: import rrweb-snapshot code #700

vlad-mh · 2021-01-22T14:49:44Z

This PR removes the dependency on rrweb-snapshot and instead adds it's code directly, changing the imports accordingly.

codecov-io · 2021-01-22T14:51:12Z

Codecov Report

Merging #700 (418a14d) into master (8dea89b) will decrease coverage by 1.67%.
The diff coverage is 58.06%.

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   79.84%   78.17%   -1.68%     
==========================================
  Files          69       71       +2     
  Lines        3433     3743     +310     
  Branches      732      887     +155     
==========================================
+ Hits         2741     2926     +185     
- Misses        692      817     +125

Impacted Files	Coverage Δ
packages/rum-recorder/src/domain/rrweb/mutation.ts	`22.42% <ø> (ø)`
packages/rum-recorder/src/domain/rrweb/observer.ts	`55.02% <ø> (ø)`
packages/rum-recorder/src/domain/rrweb/record.ts	`60.68% <ø> (ø)`
packages/rum-recorder/src/domain/rrweb/types.ts	`100.00% <ø> (ø)`
packages/rum-recorder/src/domain/rrweb/utils.ts	`41.34% <ø> (ø)`
packages/rum-recorder/src/types.ts	`100.00% <ø> (ø)`
...rum-recorder/src/domain/rrweb-snapshot/snapshot.ts	`57.09% <57.09%> (ø)`
...es/rum-recorder/src/domain/rrweb-snapshot/types.ts	`100.00% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dea89b...418a14d. Read the comment docs.

vlad-mh · 2021-01-27T09:29:24Z

packages/rum-recorder/src/domain/rrweb-snapshot/snapshot.ts

@@ -0,0 +1,589 @@
+/* eslint-disable no-underscore-dangle */


While I removed some (e.g: I renamed _id to nextId), others appeared to be a more dangerous change (e.g attributes._cssText here: https://github.com/DataDog/browser-sdk/pull/700/files#diff-1c7cef52f64a686ba523420f60390086b7a16d8f13b51ac8f0a3e79d90be827eR224)

vlad-mh · 2021-01-27T09:45:47Z

packages/rum-recorder/src/domain/rrweb-snapshot/snapshot.ts

@@ -0,0 +1,589 @@
+/* eslint-disable no-underscore-dangle */
+/* eslint-disable @typescript-eslint/prefer-regexp-exec */


I wasn't sure on this.

Example: https://github.com/DataDog/browser-sdk/pull/700/files#diff-1c7cef52f64a686ba523420f60390086b7a16d8f13b51ac8f0a3e79d90be827eR349

While replacing a 'foo'.match() to /foo/.exec() makes sense, here we're talking about creating a new regexp from the return value of lowerIfExists?

I think it simply means to change

lowerIfExists(sn.attributes.name).match(/^msapplication-tile(image|color)$/)

to

/^msapplication-tile(image|color)$/.exec(lowerIfExists(sn.attributes.name))

also we could take the opportunity to change exec to test when the result is used as a boolean

/^msapplication-tile(image|color)$/.test(lowerIfExists(sn.attributes.name))

ah, I didn't get the string & regexp were inverted between match and exec. I'll make the change

vlad-mh · 2021-01-27T09:53:23Z

@BenoitZugmeyer this should be ready for review 🙏

LICENSE-3rdparty.csv

packages/rum-recorder/src/domain/rrweb-snapshot/types.ts

bcaudan · 2021-01-27T13:49:09Z

packages/rum-recorder/src/domain/rrweb-snapshot/snapshot.ts

+const URL_IN_CSS_REF = /url\((?:(')([^']*)'|(")([^"]*)"|([^)]*))\)/gm
+const RELATIVE_PATH = /^(?!www\.|(?:http|ftp)s?:\/\/|[A-Za-z]:\\|\/\/).*/
+const DATA_URI = /^(data:)([^,]*),(.*)/i
+export function absoluteToStylesheet(cssText: string | null, href: string): string {


What about also importing unit tests?
cf https://github.com/rrweb-io/rrweb-snapshot/blob/master/test/snapshot.test.ts

fair enough, although this adds a dependency on jsdom and chai, are we comfortable with that? I know @BenoitZugmeyer wanted to look at importing rrweb tests in another PR

Yes, I'm working on it. We should not import chai or jsdom though:

jsdom is used when running tests in Nodejs (through Jest). Since we use Karma, we can just use the real DOM

chai is an assertion library, karma-jasmine provides one out of the box

I think importing snapshot unit tests would be really straightforward as there is no snapshots or complex asynchronous cases. It could be worth it to import and adjust them (to use karma/jasmine) here.

bcaudan · 2021-01-27T13:51:00Z

packages/rum-recorder/src/domain/rrweb-snapshot/snapshot.ts

+  return 'styleSheet' in rule
+}
+
+function extractOrigin(url: string): string {


could we replace it by getOrigin(url: string)?

getOrigin seem more complicated (builds an a element with buildUrl) compared to this implementation.
I'm not comfortable enough with JS/TS to guarantee the behavior is the same, so I'd rather not touch the code atm 🤔

sure, it could worth looking into that in a separate task though

IMO for now it would make sense to keep the current implementation. As @vlad-mh stated, it is simpler and may be faster than getOrigin. Using it to transform big stylesheets seems a bit perf critical. Agreed to look into it in a future task.

bcaudan · 2021-01-27T13:55:13Z

packages/rum-recorder/src/domain/rrweb-snapshot/snapshot.ts

+  return resultingSrcsetString
+}
+
+export function absoluteToDoc(doc: Document, attributeValue: string): string {


I am wondering if we could replace that by normalizeUrl(url: string)

It could make sense, but as for https://github.com/DataDog/browser-sdk/pull/700/files/0bb750012a6e3890ce84ff315158ca404dcd5382#r565322686 I don't feel comfortable enough to make that change and guarentee I don't break things

BenoitZugmeyer · 2021-02-01T10:21:35Z

packages/rum-recorder/src/domain/rrweb-snapshot/types.ts

+
+export type SerializedNodeWithId = SerializedNode & { id: number }
+
+export type tagMap = {


This type doesn't seem to be used. Could you remove it? else, change it to TagMap

good catch! latest commit removes it

This commit removes the dependency on rrweb-snapshot and instead adds the code directly, changing the imports accordingly.

bcaudan

LGTM

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch 3 times, most recently from b080cd0 to 95115bb Compare January 27, 2021 09:27

vlad-mh commented Jan 27, 2021

View reviewed changes

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch from 95115bb to 21d0663 Compare January 27, 2021 09:50

vlad-mh marked this pull request as ready for review January 27, 2021 09:51

vlad-mh requested a review from a team as a code owner January 27, 2021 09:51

BenoitZugmeyer reviewed Jan 27, 2021

View reviewed changes

LICENSE-3rdparty.csv Show resolved Hide resolved

packages/rum-recorder/src/domain/rrweb-snapshot/types.ts Outdated Show resolved Hide resolved

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch 2 times, most recently from e9a1591 to 0bb7500 Compare January 27, 2021 11:19

bcaudan reviewed Jan 27, 2021

View reviewed changes

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch from cd9783f to c724d01 Compare January 27, 2021 16:52

BenoitZugmeyer approved these changes Feb 1, 2021

View reviewed changes

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch 2 times, most recently from 2a462e2 to 4a869a2 Compare February 2, 2021 09:22

BenoitZugmeyer approved these changes Feb 2, 2021

View reviewed changes

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch from 4a869a2 to 418a14d Compare February 2, 2021 10:53

vlad-mh added 8 commits February 2, 2021 11:55

rum-recorder: import rrweb-snapshot code

a45ba5b

This commit removes the dependency on rrweb-snapshot and instead adds the code directly, changing the imports accordingly.

licenses: update 3p following embedding of rrweb-snapshot

35d2e47

rum-recorder: remove unused code in rrweb-snapshot

fbd9c56

rum-recorder: address eslint issues

ef5deae

rum-recorder: capitalize first letter of types

51afe10

rum-recorder: replace string.match() by RegExp.test()

91802c5

rum-recorder: add rrweb-snapshot unit tests

1fa9891

rum-recorder: remove unused types

a1a317f

vlad-mh force-pushed the vlad/embed-rrweb-snapshot branch from 418a14d to a1a317f Compare February 2, 2021 10:55

bcaudan approved these changes Feb 2, 2021

View reviewed changes

vlad-mh merged commit 3e204dc into master Feb 2, 2021

dependabot bot mentioned this pull request Mar 16, 2021

Bump @datadog/browser-rum from 1.13.0 to 2.6.1 in /frontend NomadBlacky/mmsf-hub#77

Closed

bcaudan deleted the vlad/embed-rrweb-snapshot branch May 5, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rum-recorder: import rrweb-snapshot code #700

rum-recorder: import rrweb-snapshot code #700

vlad-mh commented Jan 22, 2021

codecov-io commented Jan 22, 2021 •

edited

Loading

vlad-mh Jan 27, 2021

vlad-mh Jan 27, 2021

BenoitZugmeyer Jan 27, 2021

vlad-mh Jan 27, 2021

vlad-mh commented Jan 27, 2021

bcaudan Jan 27, 2021

vlad-mh Jan 27, 2021

BenoitZugmeyer Jan 27, 2021

BenoitZugmeyer Jan 27, 2021

bcaudan Jan 27, 2021

vlad-mh Jan 27, 2021

bcaudan Jan 27, 2021

BenoitZugmeyer Jan 27, 2021

bcaudan Jan 27, 2021

vlad-mh Jan 27, 2021

BenoitZugmeyer Feb 1, 2021

vlad-mh Feb 1, 2021

bcaudan left a comment

		@@ -0,0 +1,589 @@
		/* eslint-disable no-underscore-dangle */
		/* eslint-disable @typescript-eslint/prefer-regexp-exec */


		export type SerializedNodeWithId = SerializedNode & { id: number }

		export type tagMap = {

rum-recorder: import rrweb-snapshot code #700

rum-recorder: import rrweb-snapshot code #700

Conversation

vlad-mh commented Jan 22, 2021

codecov-io commented Jan 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-mh commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcaudan left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 22, 2021 •

edited

Loading