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

Pointer type fixups #1201

Merged
merged 7 commits into from
Apr 13, 2023
Merged

Conversation

eoghanmurray
Copy link
Contributor

These should have been part of #1129 as I didn't realize the tests were failing

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2023

🦋 Changeset detected

Latest commit: 4939c00

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

This PR includes changesets to release 0 packages

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

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

… ended up as a TouchEvent instead of an individual Touch object I think
@@ -1847,7 +2135,8 @@ exports[`record integration tests can record form interactions 1`] = `
\\"data\\": {
\\"source\\": 2,
\\"type\\": 1,
\\"id\\": 27
\\"id\\": 27,
\\"pointerType\\": 0
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we can hide this property by default if the pointer type is mouse.
On the one hand, it can keep most of the data unchanged. On the other hand, most data is triggered by mouse so it can help reduce event size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I've merged the pull request as this is a fixup for #1129 which is already merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hide this property by default if the pointer type is mouse

I thought of that and am still open to it, some considerations:

  • it's also possible (older browsers I guess) that the PointerType info missing, in which case it will be absent (if we went with your suggestion then we could add a PointerTypes.Unknown for this case)
  • we don't tend to do this for other events ... for example at Statcounter I measured the most common fields/values and by far the worst offender in terms of space was Mutations, so we stripped out the data.source and empty attributes/removes/adds/texts on those for the biggest win. TouchUp/Down/MouseUp/Down/Click events are hardly as common as scroll events etc.

Copy link
Contributor Author

@eoghanmurray eoghanmurray Apr 13, 2023

Choose a reason for hiding this comment

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

I guess I could remove pointerType from MouseDown/MouseUp/TouchDown/TouchUp as these are implicit, have done so in a new PR: #1206

Copy link
Contributor Author

@eoghanmurray eoghanmurray Apr 13, 2023

Choose a reason for hiding this comment

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

After #1206 the pointerType is only on Click events .... I'm also thinking that "most data is triggered by mouse" is an incorrect assumption, as 60% of web browsing is from mobile devices:
https://gs.statcounter.com/platform-market-share/desktop-mobile-tablet

@eoghanmurray eoghanmurray merged commit f884711 into rrweb-io:master Apr 13, 2023
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