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

fix: safe pause #1461

Closed
wants to merge 3 commits into from
Closed

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Apr 26, 2024

#1432 introduces this class/method.
Relates to PostHog/posthog#21874
We also saw this issue in our error reporting tool.

Not sure this is the right fix since I cannot reproduce it locally but def. happened a few times in the latest alpha on Chrome 124 Win >= 10.

Copy link

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest commit: 1181b47

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension Patch
rrvideo Patch

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

@marandaneto
Copy link
Contributor Author

cc @Juice10

@eoghanmurray
Copy link
Contributor

If it's complaining about RRMediaElement, then the fix likely should be in rrdom?

I don't understand the abstract base class stuff too well, but maybe it just needs a concrete implementation of the pause function here:
https://github.com/rrweb-io/rrweb/blob/master/packages/rrdom/src/index.ts#L152
?

@marandaneto
Copy link
Contributor Author

If it's complaining about RRMediaElement, then the fix likely should be in rrdom?

I don't understand the abstract base class stuff too well, but maybe it just needs a concrete implementation of the pause function here: master/packages/rrdom/src/index.ts#L152 ?

public pause() {
this.paused = true;
}
should be the implementation since the class extends the base class with the method implementation, so no, I don't think so but not a JS/TS expert either.

@Juice10
Copy link
Contributor

Juice10 commented May 1, 2024

Any chance we could figure out what the type of element is that's being sent through? I'd like us to have a deeper fix if possible.

@Juice10
Copy link
Contributor

Juice10 commented May 1, 2024

@marandaneto I'd prefer this fix although if we don't know exactly what's triggering it, it might not be helpful for your use case. #1462 Any chance you could check?

@marandaneto
Copy link
Contributor Author

Any chance we could figure out what the type of element is that's being sent through? I'd like us to have a deeper fix if possible.

I'd love to, but I could not reproduce it, nor were the bug reports specific enough since it's an internal method.

@marandaneto
Copy link
Contributor Author

@marandaneto I'd prefer this fix although if we don't know exactly what's triggering it, it might not be helpful for your use case. #1462 Any chance you could check?

I could not reproduce the issue so I cannot check it but your PR makes sense to me, I added a comment with a possible improvement though, I think this is worth the shot.

@marandaneto
Copy link
Contributor Author

closing in favor of #1462
cc @daibhin

@marandaneto marandaneto closed this May 2, 2024
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.

3 participants