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

[REPLAY-164] track Focus records #707

Merged
merged 7 commits into from
Feb 1, 2021
Merged

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Add focus information to always show the currently focused view during replay

Changes

  • adjust types
  • adjust rrweb architecture a bit to externalize records generation: the goal is to avoid implementing new records in the RRWeb codebase, to be more modular and easier to test. In a future PR, I'll remove some records from RRWeb, limiting its usage to recording DOM changes.
  • add Focus records tracking

Testing

Unit tests, manual tests


I have gone over the contributing documentation.

This makes things less confusing by removing `Event` and `EventTypes`
types altogether, replacing them with proper `Record` and `RecordTypes`.
This will help adding new record types.
We'll want to emit records without adding more code to RRWeb. Thus, move
the wraping utility to our code.
@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner January 29, 2021 10:14
@BenoitZugmeyer BenoitZugmeyer changed the title Benoit/add focus records [REPLAY-164] track Focus records Jan 29, 2021
Copy link
Contributor

@vlad-mh vlad-mh left a comment

Choose a reason for hiding this comment

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

I like that we're formalizing the "Record" name instead of "Event".
I see we're sending an event when focus is gained, but not lost. Is this a limitation brought by the browser APIs?

@BenoitZugmeyer
Copy link
Member Author

BenoitZugmeyer commented Jan 29, 2021

@vlad-mh

I see we're sending an event when focus is gained, but not lost. Is this a limitation brought by the browser APIs?

We are sending an event when the focus is lost too (blur). Did you experience this yourself?

Copy link
Contributor

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

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

LGTM

incrementalSnapshotCount += 1
const exceedCount = checkoutEveryNth && incrementalSnapshotCount >= checkoutEveryNth
const exceedTime = checkoutEveryNms && e.timestamp - lastFullSnapshotEvent.timestamp > checkoutEveryNms
const exceedTime = checkoutEveryNms && Date.now() - lastFullSnapshotRecordTimestamp > checkoutEveryNms
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you changed from e.timestampto Date.now()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because rrweb/record uses RawRecords instead of Records now, so records don't have the timestamp property anymore. They are "wraped" later in the recorder code. See the whole commit for context 995f863

@vlad-mh
Copy link
Contributor

vlad-mh commented Feb 1, 2021

@vlad-mh

I see we're sending an event when focus is gained, but not lost. Is this a limitation brought by the browser APIs?

We are sending an event when the focus is lost too (blur). Did you experience this yourself?

mb, I misunderstood. all good :)

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