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

Add option to block animation on <title> tag #760

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

eoghanmurray
Copy link
Contributor

These can generate massive recordings on some websites (think scrolling title tag)

@jlalmes
Copy link
Contributor

jlalmes commented Dec 9, 2021

I am making use of the <title> in recordings. I think it's fine to offer the option to ignore changes, but maybe removing changes from recordings by default when slimDOMOptions is set to all is an unintended breaking change?

Use case - when recording SPAs I would like the <title> value to update when navigating from page to page.

@Yuyz0112
Copy link
Member

Yuyz0112 commented Dec 9, 2021

good point @jlalmes

@eoghanmurray
Copy link
Contributor Author

Great spot, thanks @jlalmes ... in light of that, maybe it should be a throttling thing?
In the mean time I'll update so that you have to explicitly turn this on, rather than including it with all as you suggested.

@eoghanmurray
Copy link
Contributor Author

Actually @jlalmes, the intention of the 'all' is to enable all slim dom options — there is also a true option which is there to enable all 'safe' options (for some definition of 'safe').
Do you think you should change to using true or is that difference non-obvious?

This also reminds me that I've got some code external to rrweb which detects Single Page App url changes, but only if they happen in tandem with a <title> tag change. If I were to integrate that in rrweb it would introduce a new event, analogous to the Meta event to record SPA navigation. (this wouldn't get recorded if just the URL changed). Would that be useful?

@jlalmes
Copy link
Contributor

jlalmes commented Dec 10, 2021

Do you think you should change to using true or is that difference non-obvious?

Good idea! I will look into that.

This also reminds me that I've got some code external to rrweb which detects Single Page App url changes, but only if they happen in tandem with a <title> tag change. If I were to integrate that in rrweb it would introduce a new event, analogous to the Meta event to record SPA navigation. (this wouldn't get recorded if just the URL changed). Would that be useful?

Yes, a PageNavigation event would be really useful I think. I imagine we would want this event to fire even if the <title> tag doesn't change, however, I'd be interested to take a look at your implementation of this & collaborate on a PR.

@eoghanmurray eoghanmurray self-assigned this Apr 5, 2024
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.

This looks good, it's missing a type, and would also be great to have a test.
Is also missing a changeset, but this is a pretty old PR so that's totally understandable. Apart from that this looks good!

packages/rrweb/src/record/index.ts Outdated Show resolved Hide resolved
Copy link

changeset-bot bot commented Jun 6, 2024

🦋 Changeset detected

Latest commit: 2887191

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

This PR includes changesets to release 8 packages
Name Type
rrweb-snapshot Patch
rrweb 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

Copy link
Contributor Author

@eoghanmurray eoghanmurray left a comment

Choose a reason for hiding this comment

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

I thought I satisfied all the requested changes, not sure how to indicate that!

@eoghanmurray
Copy link
Contributor Author

OK I guess my force push messed things up, as it can't find the commits the requested changes were attached to!

@Juice10
Copy link
Contributor

Juice10 commented Jun 9, 2024

This will fix the issues with CI not passing on this PR: #1500

@Juice10 Juice10 merged commit e08706a into rrweb-io:master Jun 13, 2024
6 checks passed
jeffdnguyen pushed a commit to pendo-io/rrweb that referenced this pull request Jul 29, 2024
* Add option to block animation on <title> tag which can generate massive recordings on some websites (think scrolling title tag)

* Add new option to slimDOMOptions type with tsdoc as suggested by Justin
jeffdnguyen pushed a commit to pendo-io/rrweb that referenced this pull request Jul 29, 2024
* Add option to block animation on <title> tag which can generate massive recordings on some websites (think scrolling title tag)

* Add new option to slimDOMOptions type with tsdoc as suggested by Justin
jxiwang pushed a commit to amplitude/rrweb that referenced this pull request Oct 22, 2024
* Add option to block animation on <title> tag which can generate massive recordings on some websites (think scrolling title tag)

* Add new option to slimDOMOptions type with tsdoc as suggested by Justin
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.

4 participants