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

[REPLAY] Add support for adoptedStyleSheets #1916

Merged
merged 14 commits into from
Jan 12, 2023

Conversation

ThibautGeriz
Copy link
Contributor

@ThibautGeriz ThibautGeriz commented Jan 4, 2023

Motivation

Following what we started on the shadow dom support, let's add support for dynamic stylesheet

Changes

  • update rum format
  • add type for adoptedStylesheets because it's added in typescript 4.8 and we're using 4.7
  • add support for adoptedStylesheets when serializing on Document & ShadowRoot

Out of scope

  • Mutation of the adoptedStylesheets property
  • Mutation of CSSStylesheet themselves.

Testing

  • Local
  • Staging
  • Unit
  • End to end

Document

image

Shadow root


I have gone over the contributing documentation.

@ThibautGeriz ThibautGeriz changed the title [REPLAY] [REPLAY] Add support for adoptedStyleSheets Jan 4, 2023
@ThibautGeriz ThibautGeriz force-pushed the thibaut.gery/support-adopted-stylesheet branch from 867494e to 8ee122f Compare January 4, 2023 13:54
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #1916 (2ebb758) into main (ac26b8d) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1916      +/-   ##
==========================================
- Coverage   93.21%   93.19%   -0.03%     
==========================================
  Files         141      141              
  Lines        5349     5362      +13     
  Branches     1202     1207       +5     
==========================================
+ Hits         4986     4997      +11     
- Misses        363      365       +2     
Impacted Files Coverage Δ
packages/rum-core/test/createIsolatedDom.ts 100.00% <ø> (ø)
packages/core/src/tools/browserDetection.ts 100.00% <100.00%> (ø)
...ckages/rum/src/domain/record/serializationUtils.ts 98.14% <100.00%> (+0.42%) ⬆️
packages/rum/src/domain/record/serialize.ts 92.63% <100.00%> (+0.03%) ⬆️
packages/core/src/tools/timeUtils.ts 97.43% <0.00%> (-2.57%) ⬇️
packages/rum/src/domain/record/mutationObserver.ts 95.86% <0.00%> (-0.69%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ThibautGeriz ThibautGeriz force-pushed the thibaut.gery/support-adopted-stylesheet branch 5 times, most recently from 23ffc63 to 89f9e47 Compare January 4, 2023 15:56
@ThibautGeriz ThibautGeriz force-pushed the thibaut.gery/support-adopted-stylesheet branch 3 times, most recently from b7b6208 to 026cf50 Compare January 5, 2023 14:02
@ThibautGeriz ThibautGeriz marked this pull request as ready for review January 5, 2023 16:30
@ThibautGeriz ThibautGeriz requested review from a team as code owners January 5, 2023 16:30
@ThibautGeriz ThibautGeriz force-pushed the thibaut.gery/support-adopted-stylesheet branch from 026cf50 to 2ebb758 Compare January 6, 2023 09:48
packages/rum/src/domain/record/browser.types.ts Outdated Show resolved Hide resolved
packages/core/src/tools/browserDetection.ts Outdated Show resolved Hide resolved
packages/rum-core/test/createIsolatedDom.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/serializationUtils.ts Outdated Show resolved Hide resolved
packages/rum/src/domain/record/serialize.spec.ts Outdated Show resolved Hide resolved
@ThibautGeriz ThibautGeriz changed the title [REPLAY] Add support for adoptedStyleSheets [REPLAY] Add support for adoptedStyleSheets Jan 9, 2023
@ThibautGeriz ThibautGeriz requested review from BenoitZugmeyer and a team January 9, 2023 10:02
@ThibautGeriz ThibautGeriz requested review from bcaudan and a team January 9, 2023 13:27
test/e2e/scenario/recorder/shadowDom.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder/shadowDom.scenario.ts Outdated Show resolved Hide resolved
test/e2e/scenario/recorder/shadowDom.scenario.ts Outdated Show resolved Hide resolved
Comment on lines 299 to 301
function isAdoptedStyleSheetsSupported() {
return ['edge', 'chrome'].includes(getBrowserName())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 suggestion: ‏Could we check the adoptedStyleSheets property on document instead?
it would allow to automatically run this test on other browsers when available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no because it would be in the runner (node) not in the targetted browser :(

Copy link
Contributor

Choose a reason for hiding this comment

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

browserExecute helper could execute this check on the browser side

Copy link
Contributor Author

@ThibautGeriz ThibautGeriz Jan 9, 2023

Choose a reason for hiding this comment

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

Indeed, @BenoitZugmeyer you suggest that approach. Maybe you want to share your opinion here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to get the result out of the browser to the runner to stop the test?

Copy link
Member

@BenoitZugmeyer BenoitZugmeyer Jan 9, 2023

Choose a reason for hiding this comment

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

It's a bit more annoying as it would be asynchronous, but we can try!

Note, instead of if (...) { createTest(...).run() } it would need to done in the .run() callback like:

.run(async () => {
  if (!await isAdoptedStyleSheetsSupported()) { pending('...') }
  ...
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BenoitZugmeyer How can I test that locally? By default it will run on Chrome up to date and be ok with the test but how can I test that it will be pending?

Copy link
Member

Choose a reason for hiding this comment

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

You can try with a constant condition (ex: if (!await Promise.resolve(false)) { pending() }

About the condition specifically, I managed to run e2e tests in firefox locally, or you can run your tests on browserstack with the yarn test:e2e:bs with the right tokens as environment variable. Or simply push the change and see how it goes :)

packages/rum/src/domain/record/serializationUtils.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ThibautGeriz ThibautGeriz left a comment

Choose a reason for hiding this comment

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

[DELETED]

@ThibautGeriz ThibautGeriz requested a review from bcaudan January 11, 2023 07:54
@ThibautGeriz ThibautGeriz merged commit 28e178b into main Jan 12, 2023
@ThibautGeriz ThibautGeriz deleted the thibaut.gery/support-adopted-stylesheet branch January 12, 2023 09:04
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.

4 participants