-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix duplicated shadow dom #1095
Conversation
clean observers before taking new full snapshots
Hey @Mark-Fenng, why do you mean by taking fullsnapshot manually? and why are there duplicated nodes? Do you mean taking fullsnapshot by manually starting the recording again (i.e. after rrweb has already taken a full-snapshot)? Why do we need this workaround then? Shouldn't be we cleaning up all the observers in that case? |
Taking fullsnapshot manually means call rrweb.takeFullSnapshot(true) after the recorder started. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for applying suggestions. The PR looks great to me!
JSON.stringify(a.attributes) === | ||
JSON.stringify((b as elementNode).attributes) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a slow call to do, aren't you afraid this will slow down repeated rrweb.takeFullSnapshot(true)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what happens if a node gets a new attribute since the last time it was snapshotted? Will this node get added twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a slow call to do, aren't you afraid this will slow down repeated
rrweb.takeFullSnapshot(true)
?
Yes you are right, this function is slow. It's in the rebuilt.ts so I don't think this can slow down the takeFullSnapshot function. If the data events are correct, the program won't go to this path. This comparison will only be used when rrweb ingests any problematic data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what happens if a node gets a new attribute since the last time it was snapshotted? Will this node get added twice?
If the recorder works well, the incremental data will only change the node's attribute rather than totally recreate the node through this path.
In case the program goes here, the node will get added twice as you said. But honestly, I don't know what kind of error event data can lead to this. I wrote this function as a fallback.
Do you have any suggestions for improving the code here?
🦋 Changeset detectedLatest commit: 08e4b1c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
Bug
There are piles of duplicated nodes after a recorder takes a new full snapshot manually.
Duplicated node creation may cause unexpected errors in the replayer.
Fix