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

Preserve iframe node id through loading callback #724

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eoghanmurray
Copy link
Contributor

@eoghanmurray eoghanmurray commented Oct 7, 2021

Ensure the iframe id survives the onceIframeLoaded process; for some reason I'm seeing the __sn attribute on the iframe DOM element disappearing with the callback

@eoghanmurray eoghanmurray force-pushed the iframe-loses-sn branch 2 times, most recently from f766e56 to 8af21d4 Compare October 7, 2021 16:12
Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/rrweb/src/record/iframe-manager.ts Outdated Show resolved Hide resolved
…e reason I'm seeing the `__sn` attribute on the iframe DOM element disappearing with the callback
Copy link

changeset-bot bot commented Apr 5, 2024

⚠️ No Changeset found

Latest commit: 9b4459e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@eoghanmurray
Copy link
Contributor Author

So the circumstances of this PR were that the iframe element was 'somehow' losing it's __sn DOM attribute.

We no longer store the __sn in the DOM but rather retrieve the id from the mirror.

So now the question would be whether the identity of the iframe has changed such that in that scenario, the mirror wouldn't be able to retrieve it. I don't know as I don't have the scenario available anymore.

So we could merge as a precaution; what do you think @Juice10 ?

@Juice10
Copy link
Contributor

Juice10 commented Apr 8, 2024

@eoghanmurray we don't have a test case for this. And the underlying code has changed pretty heavily. I'm thinking we close this till it becomes a problem again and resurrect it then (with test case). But I'm not 100% on this one

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.

2 participants