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

Handle negative ids in rrdom correctly + extra tests #927

Merged
merged 34 commits into from
Aug 6, 2022

Conversation

Juice10
Copy link
Contributor

@Juice10 Juice10 commented Jul 1, 2022

When iframes get inserted they create untracked elements, both on the dom and rrdom side.
Because they are untracked they generate negative numbers when fetching the id from mirror.
This creates a problem when comparing and fetching ids across mirrors.
This PR tries to get away from using negative ids as much as possible in rrdom's comparisons.

PR also adds more test coverage for inlining images.
Also fixes recording of iframe mutations once the page gets reloaded

Juice10 added 28 commits May 31, 2022 18:09
When iframes get inserted they create untracked elements, both on the dom and rrdom side.
Because they are untracked they generate negative numbers when fetching the id from mirror.
This creates a problem when comparing and fetching ids across mirrors.
This commit tries to get away from using negative ids as much as possible in rrdom's comparisons
@Juice10 Juice10 changed the title Handle negative ids in rrdom correctly Handle negative ids in rrdom correctly + extra tests Jul 1, 2022
@YunFeng0817
Copy link
Member

Is this PR also a fix for #917?
I also encountered the problem caused by negative ids and this fix is really nice!

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

The negative ids part looks good to me.

packages/rrdom/src/diff.ts Outdated Show resolved Hide resolved
@Juice10
Copy link
Contributor Author

Juice10 commented Jul 25, 2022

Is this PR also a fix for #917?

@Mark-Fenng I don't think so, I tried replaying the JSON Blobs on my branch and on rrwebdebug and they look the same.

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 25, 2022

I set the first unserialised id to be -2 so its clear when an id gets returned from the mirror if it is a mirror miss or an unserialised id

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

Congrats, you figured out the method to fix the test failure. Your solution also looks good.

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 26, 2022

Thanks @Mark-Fenng!

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.

Nice patch! Let's merge this after a rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants