Add Asset event type and capture assets #1239
Conversation
🦋 Changeset detectedLatest commit: c160f50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
I'm wondering whether it's worth taking this commit out and merging in a separate PR?
Fine if not; maybe in future this sort of thing could be merged in earlier while rest of a PR is ongoing.
If we weren't on an alpha release that type of commit would also require us to do a major version change. Not sure if it makes sense now, it's bound to break some tests and those got fixed later in subsequent commits. But in general I do agree with you, this type of thing would be better in a separate PR |
| return Promise.all(promises); | ||
| } | ||
|
|
||
| public reset(config?: captureAssetsParam | undefined): void { |
There was a problem hiding this comment.
Why do we have to reset when a meta event is captured?
There was a problem hiding this comment.
This is for live-mode/addEvent. Once a meta event is triggered its an indication that a page navigation has happened. That tells us that we don't have to keep waiting for new asset events to show up, hence the reset being called.
There was a problem hiding this comment.
I see. But do we need to store the capture config in the meta event, and reload it as this.config = config when a meta event is emitted?
Or we can think the capture config is the same across a session and also call reset when a meta event is emitted, but without storing a copy of the capture params in the meta event?
There was a problem hiding this comment.
Verdict of our conversation:
- I'll move over the captureAssetsParam for the replay from the meta event to the full snapshot.
- Copy dimensions from meta to full snapshot.
| }; | ||
|
|
||
| this.capturedURLs.add(url); | ||
| this.capturingURLs.delete(url); |
There was a problem hiding this comment.
how about move this line and the same line in the error block into a finally block?
|
Just reviewed the last two files: the asset manager observer and the replay side manager, really cool implementation! Leave some small comments, and we should be able to merge this in the following days. |
|
Not sure if this is a goal or a non-goal, but should we be collecting all assets for the |
| private async preloadAllAssets(timestamp: number): Promise<void[]> { | ||
| const promises: Promise<void>[] = []; | ||
| for (const event of this.service.state.context.events) { | ||
| if (event.timestamp <= timestamp) continue; |
There was a problem hiding this comment.
So we are ignoring old asset events ... is the idea here to get the 'most recent version' of an asset?
Aside: is there any risk that the timestamp of the asset event will be the same as the fullsnapshot timestamp?
eoghanmurray
force-pushed
the
asset-event
branch
2 times, most recently
from
7434552
to
a88ff5e
Compare
AssetManager
xlink:href is deprecated Most modern browsers rewrite `xlink:href` to `href` which is currently supported Tested in Firefox, Safari & Chrome
"diff algorithm for rrdom › apply virtual style rules to node › should insert rule at index [0,0] and keep existing rules"
AssetManager.manageAttribute
different types of assets (not just img#src)
Co-authored-by: Eoghan Murray <[email protected]>
…the AssetManager needs to interrogate other attributes to determine how to manage, e.g. <link with href depending on rel="stylesheet"
Asset events allow for asynchronous events containing (image or potentially other) assets.
See #860 for more info.
These are async events that get emmited after the original dom mutation events and can be used to inject these assets after the fact in the player or via post processing.
Todo:
[ ] add flag to log assets being captured[ ] RemoveinlineImagesin favour ofassetCaptureinlineImagesconfig toassetCaptureyarn live-streamsrcattributes while asset is being awaitedalso update asset manager to work with setAttributeNSsetAttributeNS was needed forxlink:href, but all modern browsers rewrite that tohrefwhich is already supported out of the box.srcsetreplay/asset/index.tstoreplay/asset-manager/index.ts