-
Notifications
You must be signed in to change notification settings - Fork 144
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-1517] split domain utils #2105
Conversation
let nextId = 1 | ||
|
||
return { | ||
get(event: Event): number { |
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: Why not keeping this a singleton?
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.
Looking at it, it was not utility code but domain logic, so following the convention, I passed it as a dependency rather than statically import it.
By automatism, I switched the singleton getter to an object but indeed we could keep the singleton getter and pass it as a dependency.
Any pros/cons to favour the singleton?
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.
I'm a bit surprised to see this kind of change included in a PR focused on splitting files. I have mixed feelings on enforcing the "dependency as parameter" policy when it's not really needed, but fine.
Still, I think the getRecordIdForEvent
name was clearer than recordIds.get
. Maybe we could rename it recordIds.getIdForEvent
?
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.
Maybe we could rename it recordIds.getIdForEvent?
LGTM
I have mixed feelings on enforcing the "dependency as parameter" policy when it's not really needed, but fine.
We should chat about that to see if we can get aligned
Codecov Report
@@ Coverage Diff @@
## main #2105 +/- ##
==========================================
- Coverage 93.67% 93.55% -0.12%
==========================================
Files 163 164 +1
Lines 5691 5694 +3
Branches 1303 1303
==========================================
- Hits 5331 5327 -4
- Misses 360 367 +7
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Motivation
Avoid single utils files
Changes
Remove
record/utils
andrecord/observers/utils
by:getPathToNestedCSSRule
tostyleSheetObserver
forEach
andlistenerObserver
tocore/tools/utils
(for future specialisation)record/assembly
(observer -> record assembly -> emit
design could later evolve to match what we have for rum and logs events)eventsUtils
recordIds
Testing
I have gone over the contributing documentation.