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

Save images offline, in the snapshot #770

Merged
merged 6 commits into from
Jan 11, 2022
Merged

Conversation

croqaz
Copy link
Contributor

@croqaz croqaz commented Dec 14, 2021

Hi guys,

I made this feature that I already discussed here: #737
It's about saving images inside the snapshot as base64, just like we have for canvas already.

I tried to use the same code style and not change things too much.
I added a test, but I'm not sure if it's in the right place. In the integration, it seemed the best place, because you're using Puppeteer and we need to simulate a canvas to make it work.

Please let me know if you think something is wrong, or you want more tests, or I'm missing something.

const image = (node as HTMLImageElement);
if (!image.currentSrc.startsWith('data:')) {
// backup original img src
image.setAttribute('data-src', image.currentSrc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I used the data-src attribute for keeping the original URL.
Maybe we can use something else, less likely to conflict, maybe data-rr-src ? or data-rrweb-src ?
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I just went ahead and renamed this to data-rrweb-src.

Copy link
Member

@Yuyz0112 Yuyz0112 left a comment

Choose a reason for hiding this comment

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

Very cool feature! Thanks!

guide.md Outdated
@@ -155,6 +155,7 @@ The parameter of `rrweb.record` accepts the following options.
| packFn | - | refer to the [storage optimization recipe](./docs/recipes/optimize-storage.md) |
| sampling | - | refer to the [storage optimization recipe](./docs/recipes/optimize-storage.md) |
| recordCanvas | false | whether to record the canvas element |
| recordImages | false | whether to record the image content |
Copy link
Member

Choose a reason for hiding this comment

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

maybe inlineImages or something more concrete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done!

@@ -496,6 +512,15 @@ function serializeNode(
if (tagName === 'canvas' && recordCanvas) {
attributes.rr_dataURL = (n as HTMLCanvasElement).toDataURL();
}
// save image offline
if (tagName === 'img' && recordImages && canvasService && canvasCtx) {
Copy link
Member

Choose a reason for hiding this comment

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

To avoid some corner cases in the browser, especially some security options with cross-origin domains, how about adding a try-catch to this block? So even though corner cases happen, it will not block the recording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understood where you want the try catch, I added it a little below.
See cec7fda#diff-6434d77e679a3d1e4217dcad03ee6f02551e41dc0b077a06a45020e68e1b4ee3R519-R526

canvasCtx.drawImage(image, 0, 0);
attributes.rr_dataURL = canvasService.toDataURL();
} catch {
// ignore error
Copy link

Choose a reason for hiding this comment

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

shall we log something here, e.g. with console.warn?

Copy link
Member

Choose a reason for hiding this comment

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

Since this happened on the record side, at least we need an option to do a log.

And this is out of the scope of this PR, feel free to open a new one! Thanks.

@Yuyz0112
Copy link
Member

Sorry for the late, let's go!

@Yuyz0112 Yuyz0112 merged commit 69a1b9f into rrweb-io:master Jan 11, 2022
@croqaz croqaz mentioned this pull request Jan 12, 2022
@eoghanmurray
Copy link
Contributor

@croqaz could you check out #1468 for me? This builds on top of subsequent work in #822

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