♻️ Remove automatic snake case #699
Conversation
| count: action.counts.longTaskCount, | ||
| }, | ||
| resource: { | ||
| count: action.counts.resourceCount, | ||
| }, | ||
| }, | ||
| } | ||
| } as Partial<RawRumActionEvent>) |
There was a problem hiding this comment.
I've added it because ts was not complaining with the old format.
It compiles without it but then autoActionProperties could be anything.
wdyt?
There was a problem hiding this comment.
FMU, the issue was because loadingTime and longTask are optional properties. What about using an approach similar to https://medium.com/terria/typescript-transforming-optional-properties-to-required-properties-that-may-be-undefined-7482cb4e1585 ? I guess it could be useful at other places too.
diff --git packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts
index 201f0f9f..08a712f7 100644
--- packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts
+++ packages/rum-core/src/domain/rumEventsCollection/action/actionCollection.ts
@@ -22,10 +22,13 @@ export function startActionCollection(lifeCycle: LifeCycle, configuration: Confi
}
}
+type Complete<T> = {
+ [P in keyof Required<T>]: Pick<T, P> extends Required<Pick<T, P>> ? Complete<T[P]> : Complete<T[P]> | undefined
+}
+
function processAction(action: AutoAction | CustomAction) {
const autoActionProperties = isAutoAction(action)
- ? // tslint:disable-next-line: no-object-literal-type-assertion
- ({
+ ? {
action: {
error: {
count: action.counts.errorCount,
@@ -39,10 +42,10 @@ function processAction(action: AutoAction | CustomAction) {
count: action.counts.resourceCount,
},
},
- } as Partial<RawRumActionEvent>)
+ }
: undefined
const customerContext = !isAutoAction(action) ? action.context : undefined
- const actionEvent: RawRumActionEvent = combine(
+ const actionEvent: Complete<RawRumActionEvent> = combine(
{
action: {
target: {There was a problem hiding this comment.
I don't think that the issue is related to optional properties, for ex with:
action: {
foo: 'bar',
error: {
count: action.counts.errorCount,
},
...
}I would expect a ts issue since foo is not defined on RawRumActionEvent but it is not the case.
Anyway, since the automatic snake case has been removed, this error is as least caught by unit tests.
I've removed the explicit casting.
Co-authored-by: Benoît <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #699 +/- ##
==========================================
+ Coverage 79.58% 79.60% +0.02%
==========================================
Files 68 68
Lines 3448 3433 -15
Branches 737 737
==========================================
- Hits 2744 2733 -11
+ Misses 704 700 -4
Continue to review full report at Codecov.
|
* ♻️ replace camelCase by snake_case for RawRumEvents * 🔥 remove snake case conversion * 👌 do not remap view properties Co-authored-by: Benoît <[email protected]> * 👌 remove cast Co-authored-by: Benoît <[email protected]>
This reverts commit f77ac21.
Motivation
Our properties need to be snake cased but not customer properties.
Automatic conversion + exception for customer properties can be error prone if we forget to add exception when we add new customer properties.
Changes
Testing
Unit
I have gone over the contributing documentation.