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 userTriggered #495

Merged
merged 10 commits into from
Jul 8, 2021
Merged

Add userTriggered #495

merged 10 commits into from
Jul 8, 2021

Conversation

Juice10
Copy link
Contributor

@Juice10 Juice10 commented Feb 18, 2021

What does userTriggered indicate?

userTriggered gets added to input events that indicates if this event was triggered directly by the user or not.
Example of userTriggered in action: User clicks on radio element (userTriggered: true) which triggers the other radio element to change (userTriggered: false)

Background:

When analysing RRweb events I was having a hard time trying to figure out if an event was directly generated by a user or not. This PR adds a notation to indicate wherever this was the case or not.

Naming considerations: I was doubting if userTriggered or isTrusted was the right name for this attribute. But since isTrusted doesn't completely cover all the cases I thought userTriggered might be a better name.

@Yuyz0112
Copy link
Member

Thanks, @Juice10 !

Things I'm thinking about

  1. So we can put more things into userTriggered when we have things other than isTrusted?
  2. Although it's just a boolean flag to some events, it's still something not related to record and replay, because it does not have a visual effect. It's totally okay to add this property, but I'm wondering whether we should implement a plugin system so only users who use the userTriggeredPlugin will have this data collected. This may help us have a minimal data structure and flexible extensions.

@Juice10
Copy link
Contributor Author

Juice10 commented Feb 19, 2021

Thanks, @Juice10 !

Things I'm thinking about

  1. So we can put more things into userTriggered when we have things other than isTrusted?

Because this also covers events for radio buttons that get automatically get unselected when you click on a different radio button. I believe technically ‘isTrusted’ would be true in this case. (But I could be wrong and then ‘isTrusted’ would be a perfect name for it.)

  1. Although it's just a boolean flag to some events, it's still something not related to record and replay, because it does not have a visual effect. It's totally okay to add this property, but I'm wondering whether we should implement a plugin system so only users who use the userTriggeredPlugin will have this data collected. This may help us have a minimal data structure and flexible extensions.

Could you elaborate a bit on how you’d like the plug-in system to work? I’m guessing you’d want something more elaborate than a simple config flag to toggle this data collection on and off.

@eoghanmurray
Copy link
Contributor

it's still something not related to record and replay, because it does not have a visual effect

I'd say that in the context of a replayer, there may be cases where you'd want to have a visual effect when you know an event was performed by the user vs. otherwise.

@eoghanmurray
Copy link
Contributor

@Yuyz0112 I talked to Justin about this today.
IMO this should be a config switch rather than a plugin.
I think plugins should be reserved for large features which would otherwise bulk out the .min.js payload if they were a config option.
E.g. with #598 would src/record/stringify.ts and initLogObserver code be excluded from the packed js if the replayConsole plugin is not included?

@Yuyz0112
Copy link
Member

Yuyz0112 commented Jul 2, 2021

@eoghanmurray

Agree! I've found this feature needs much tiny modification in the core code path, so it's not a good idea to be a plugin.

@Juice10 Could you add an option for this feature?

BTW, I think #598 already helps prune the stringify/initLogObserver code from the main package.

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 2, 2021

@Yuyz0112 I'd be happy to, will let you know when its done!

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 6, 2021

@eoghanmurray @Yuyz0112 I added userTriggered behind the userTriggeredOnInput: true flag in the config and added tests and documentation for it as well.

@Yuyz0112
Copy link
Member

Yuyz0112 commented Jul 7, 2021

The code looks good to me. Not sure why there is some code diff from other commits.

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 7, 2021

@Yuyz0112 ah I think that's because I did a git rebase. I can create a new PR which won't have the excessive diff if you'd like. I can fiddle with the PR to fix it

@Juice10 Juice10 changed the base branch from master to canvas July 7, 2021 11:56
@Juice10 Juice10 changed the base branch from canvas to master July 7, 2021 11:56
@Juice10
Copy link
Contributor Author

Juice10 commented Jul 7, 2021

@Yuyz0112 fixed it, the diff should be correct now on this PR

@Yuyz0112
Copy link
Member

Yuyz0112 commented Jul 8, 2021

@Juice10 So we only apply this to input event, but not to other events like mouse interactions?

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 8, 2021

@Yuyz0112 yes thats correct

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 8, 2021

@Yuyz0112 Im happy to implement this for mouse as well later on, once I've found some good ways of replicating it.

@Yuyz0112 Yuyz0112 merged commit 7a0e04c into rrweb-io:master Jul 8, 2021
@@ -115,6 +115,7 @@ export class Replayer {
triggerFocus: true,
UNSAFE_replayCanvas: false,
pauseAnimation: true,
userTriggeredOnInput: true,
Copy link
Member

Choose a reason for hiding this comment

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

@Juice10 I found this option doesn't perform any work in the replay part. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @Mark-Fenng! It should only be on the recording end. I'm going to remove it on the replayer today.

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