-
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
[RUMF-441] Track event counts for user actions #358
Conversation
First public functions, then private function in the same order they appear in the public functions. This commit also moves the associated tests to follow the same order.
This new name is more explicit on what the function really those. In the next commit, the function will be trimed down a bit to focus on the 'wait' process.
This new `newUserAction` function uses other, self-contained functions to implement a single user action collection. The goal is to reuse this for the future 'view load' user actions.
This fix is not very elegant. I did work on a nicer fix, but it involved changing the viewTracker implementation. I may open a separate PR for this after a bit more work.
This is a workaround on the no-object-literal-type-assertion. This rule is useful in general, but in tests it can be cumbersome to mock the full objects every time. We can't disable this rule just for tests for now, but we'll be able to do so when switching to ESLint
packages/rum/src/viewTracker.ts
Outdated
const { reset } = trackEventCounts(lifeCycle, (eventCounts) => { | ||
viewMeasures = { ...viewMeasures, ...eventCounts } | ||
scheduleViewUpdate() | ||
}) | ||
resetEventCounts = reset |
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.
It could be nicer to manage the reset inside the main function by splitting trackMeasures into something like trackTimings
and trackEventCounts
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.
The issue is, the name trackEventCounts
is already used. IMO this doesn't matter too much since I'll remove this in the PR I'm working on.
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.
what I had in mind was not to create a new trackEventCounts function but to use it in the main function directly.
Since I don't know what the goal and the implications of the new PR is, it is hard to me to see if it will improve this point.
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.
Alright.
Codecov Report
@@ Coverage Diff @@
## master #358 +/- ##
==========================================
+ Coverage 86.91% 86.93% +0.02%
==========================================
Files 29 30 +1
Lines 1689 1707 +18
Branches 349 352 +3
==========================================
+ Hits 1468 1484 +16
- Misses 221 223 +2
Continue to review full report at Codecov.
|
No description provided.