-
Notifications
You must be signed in to change notification settings - Fork 142
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
♻️✅ Harmonize record observers #2659
Conversation
e81ba6d
to
5520199
Compare
Bundles Sizes Evolution
|
/to-staging |
🚂 Branch Integration: starting soon, merge in < 10m Commit 552019963a will soon be integrated into staging-12. This build is going to start soon! (estimated merge in less than 10m) Use |
…staging-12 Co-authored-by: Aymeric Mortemousque <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit 552019963a has been merged into staging-12 in merge commit 42b67d2819. Check out the triggered pipeline on Gitlab 🦊 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2659 +/- ##
==========================================
+ Coverage 92.91% 93.33% +0.42%
==========================================
Files 239 238 -1
Lines 6931 6904 -27
Branches 1517 1517
==========================================
+ Hits 6440 6444 +4
+ Misses 491 460 -31 ☔ View full report in Codecov by Sentry. |
packages/rum/src/domain/record/observers/viewportResizeObserver.ts
Outdated
Show resolved
Hide resolved
dd29d5c
to
f45226e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ question: any reason to have recordIds
under trackers
but not elementsScrollPositions
for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it was already under "observers/" which I just renamed. Do you have an improvement in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, since it is the only one not being named trackXXX
, it stands out a bit.
Compared to the others, it does not track browser events or APIs but store an internal state.
Looking at elementsScrollPositions
, it seems a bit similar.
So I am wondering if both should not be inside or outside trackers
.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion so let's put it outside to have do the minimal amount of change :)
Motivation
While implementing webview support for Session Replay, I identified inconsistencies in the recorder observers, plus some missing tests.
Changes
stop
functioninitObservers()
wrapper.Testing
I have gone over the contributing documentation.