Properly load rrweb-snapshot on AMD-based web pages #210
Conversation
andrewortwein
changed the title
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
| @@ -35,7 +35,15 @@ async function takeSnapshot( | |||
| // Serialize and capture the DOM | |||
| const domSnapshot: elementNode = await page.evaluate(dedent` | |||
| ${rrweb}; | |||
| rrwebSnapshot.snapshot(document); | |||
| if (typeof define === "function" && define.amd) { | |||
| new Promise((resolve) => { | |||
There was a problem hiding this comment.
One small thing -- is there a reason we're wrapping this in a Promise here? Since we're not awaiting the results of the promise.
There was a problem hiding this comment.
I don't quite understand why, but this doesn't work if I remove the Promise
There was a problem hiding this comment.
Maybe playwright is waiting on the promise somehow? Maybe we should return new Promise... just to make that 100% clear.
There was a problem hiding this comment.
The return is implicit. See https://playwright.dev/docs/api/class-page#page-evaluate. I find it a bit confusing, but that seems to be how it functions. Just to be sure, I tried adding return in front of new Promise and it errored with Error: page.evaluate: SyntaxError: Illegal return statement.
There was a problem hiding this comment.
I don't see how if (x) { y } else { z } evaluates to anything, but clearly it does somehow (I assume we use the return value domSnapshot still right?).
Maybe there is some way to wrap this in a function (so you can return) to make it less confusing? Otherwise I'd be in favour of a ternary (x ? y : z) to make it less weird.
There was a problem hiding this comment.
I added additional comments to hopefully point out the odd behavior that page.evaluate displays, along with a link to Playwright doc. I'd like to avoid using a ternary since it's possible we might have other scenarios to account for in the future.
| @@ -35,7 +35,15 @@ async function takeSnapshot( | |||
| // Serialize and capture the DOM | |||
| const domSnapshot: elementNode = await page.evaluate(dedent` | |||
| ${rrweb}; | |||
| rrwebSnapshot.snapshot(document); | |||
| if (typeof define === "function" && define.amd) { | |||
| new Promise((resolve) => { | |||
There was a problem hiding this comment.
I don't see how if (x) { y } else { z } evaluates to anything, but clearly it does somehow (I assume we use the return value domSnapshot still right?).
Maybe there is some way to wrap this in a function (so you can return) to make it less confusing? Otherwise I'd be in favour of a ternary (x ? y : z) to make it less weird.
Issue: #AP-5218
What Changed
Any tests using
@chromatic-com/playwrighterror if the page being tested loads modules using the AMD API (requirejs) and is running in Chrome. This change detects AMD and properly loads our fork ofrrweb-snapshotso that we can take a snapshot of the page.As part of this PR, I first added a new AMD-based page and then created a Playwright test to take a snapshot of that page. Before implementing the fix, the test failed in exactly the same way that the customer's tests are failing.
Background
We currently use
rrweb-snapshotv2.0.0-alpha.17. They made some big changes from v2.0.0-alpha.14 and v2.0.0-alpha.15. Among those changes was how modules were being exported. Something about the older way they did this worked fine, but the newer version does not. This issue might be related: rrweb-io/rrweb#1592.How to test
http://cookbook.tools.bbc.co.uk/and verify that it is captured correctly