-
Notifications
You must be signed in to change notification settings - Fork 156
♻️ [RUM-9614] Decouple default context from assembly using hooks #3498
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
Conversation
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.
Amazing
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3498 +/- ##
==========================================
+ Coverage 92.17% 92.22% +0.04%
==========================================
Files 312 313 +1
Lines 8043 8053 +10
Branches 1819 1820 +1
==========================================
+ Hits 7414 7427 +13
+ Misses 629 626 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
packages/rum-core/src/hooks.ts
Outdated
} | ||
results.push(result) | ||
} | ||
return combine(...(results as unknown as [object, object])) as ReturnType<HookCallbackMap[K]> | ||
|
||
return (results.length > 0 ? combine(...(results as unknown as [object, object])) : {}) as PartialRumEvent |
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.
thought: The non-null assertion we wanted to avoid seemed better than this {} as PartialRumEvent
cast.
With the former, we acknowledge that although triggerHook
can return undefined
in theory, it can't happen in assembly.ts
. We also assume the PartialRumEvent
is in fact a valid RUM event with all the required properties defined.
With the latter, we return an empty object, which is not only incorrect from a type perspective (PartialRumEvent
expect to have a type
property), but also doesn't improve the situation in assembly.ts
as we'll still need to implicitly assume that it won't return an empty object.
/to-staging |
View all feedbacks in Devflow UI.
Commit 3611819207 will soon be integrated into staging-19.
Commit 3611819207 has been merged into staging-19 in merge commit dfc1c59dab. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
…ing-19 Integrated commit sha: 3611819 Co-authored-by: amortemousque <[email protected]>
Motivation
Decouple the default context from the assembly. The 'default context' includes the following data:
Changes
Testing
I have gone over the contributing documentation.