ref(replay): Handle checkouts more explicitly #7321
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mydea
commented
Mar 2, 2023
| @@ -40,5 +29,16 @@ | |||
| "id": 15, | |||
| "x": 157, | |||
| "y": 90 | |||
| }, | |||
There was a problem hiding this comment.
I have no idea why this is suddenly ordered differently... It seems to be consistently so now. Not sure @cmanallen / @billyvg if you know, does this make any difference, or is the order of records in this incremental snapshot array not important? I don't really see timestamps here, but not sure...
There was a problem hiding this comment.
Hmm not sure, also interesting it doesn't have a type either
mydea
commented
Mar 2, 2023
| "rel": "icon", | ||
| "type": "image/png", | ||
| "href": "file://assets/icon/favicon.png" | ||
| "rr_width": "[0-50]px", |
There was a problem hiding this comment.
This is unrelated but wasn't updated and fails locally.
size-limit report 📦
|
mydea
force-pushed
the
fn/replay-handle-checkout
branch
from
4bbec5d to
ee92a14
Compare
Lms24
approved these changes
Mar 3, 2023
| @@ -45,21 +45,31 @@ sentryTest( | |||
|
|
|||
| const reqPromise0 = waitForReplayRequest(page, 0); | |||
| const reqPromise1 = waitForReplayRequest(page, 1); | |||
| const reqPromise2 = waitForReplayRequest(page, 2); | |||
There was a problem hiding this comment.
(no action required)
Good change, let's just always keep these at the top of tests for consistency going forward
billyvg
approved these changes
Mar 3, 2023
| @@ -40,5 +29,16 @@ | |||
| "id": 15, | |||
| "x": 157, | |||
| "y": 90 | |||
| }, | |||
There was a problem hiding this comment.
Hmm not sure, also interesting it doesn't have a type either
mydea
force-pushed
the
fn/replay-handle-checkout
branch
3 times, most recently
from
c6a8c24 to
cdb2b17
Compare
mydea
force-pushed
the
fn/replay-handle-checkout
branch
from
cdb2b17 to
c6cef1b
Compare
AbhiPrasad
pushed a commit
that referenced
this pull request
Mar 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We used to look at
event.type === 2for replay events to infer if something is a checkout or not.This is a bit hacky, as type === 2 actually just means it is a full snapshot, which may or may not be due to a checkout.
This makes it hard/impossible to do any future changes where we may want to do a full snapshot in other scenarios than in error mode, e.g. if we want to do a full snapshot to avoid large mutations.
This PR refactors this a bit to really just look at
isCheckout, plus also handle the very first rrweb event as a checkout (because we rely on this a bit).This replaces #7237