Add type checking for codecov metrics #3148
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3148 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 923 923
Lines 14400 14401 +1
Branches 3910 3910
=======================================
+ Hits 14157 14158 +1
Misses 238 238
Partials 5 5
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3148 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 923 923
Lines 14400 14401 +1
Branches 3855 3855
=======================================
+ Hits 14157 14158 +1
Misses 238 238
Partials 5 5
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3148 +/- ##
=======================================
Coverage 98.31% 98.31%
=======================================
Files 923 923
Lines 14400 14401 +1
Branches 3941 3936 -5
=======================================
+ Hits 14157 14158 +1
Misses 238 238
Partials 5 5
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 510 bytes ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Bundle ReportChanges will increase total bundle size by 510 bytes ⬆️
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3148 +/- ##
===========================================
Coverage 98.31000 98.31000
===========================================
Files 923 923
Lines 14400 14401 +1
Branches 3910 3936 +26
===========================================
+ Hits 14157 14158 +1
Misses 238 238
Partials 5 5
Continue to review full report in Codecov by Sentry.
|
| export const EVENT_METRICS = { | ||
| VISITED_PAGE: 'VISITED_PAGE', | ||
| CLICKED_BUTTON: 'CLICKED_BUTTON', | ||
| COPIED_TEXT: 'COPIED_TEXT', | ||
| COMPLETED_UPLOAD: 'COMPLETED_UPLOAD', | ||
| INSTALLED_APP: 'INSTALLED_APP', | ||
| } as const |
There was a problem hiding this comment.
Instead of creating an object here, can we just export the different metrics as const's? Using objects like this stop some minification optimizations from happening because you can't shorten names on field properties for objects, instead just exporting the const's directly.
There was a problem hiding this comment.
You can pre-face them with METRICS_ to better identify them.
There was a problem hiding this comment.
It's a tradeoff, in Rohit's approach the optimization is being traded off for a better auto-complete experience in the IDE.
There was a problem hiding this comment.
I prefer to use a single object as a source of truth for all metrics as it is more structured that way. IMO the benefits from readability and accessibility of the structure outweigh any minification optimizations on a few lines, given just a small number of constants.
Happy to make the change if you think there are any practical performance concerns from not doing this.
There was a problem hiding this comment.
Giving this a bit more thought, I think we should take a slightly different approach then both the current implementation and my original suggestion, my original reasoning isn't all that great.
I think we should remove the object entirely because it's introduces a lot of duplicate code both the key and value are always the same, and using the object that way also introduces the possibility that only one side of the key or value is changed but not the other, making this code brittle.
The approach I think would solve all the concerns and be very ergonomic is to create a union type and just use string literals in place. Right now the typeahead will only work after you imported the object, whereas with a type union it works instantly after you open a leading quote.
Here's an example of what the union type would look like:
type EventMetric = 'VISITED_PAGE' | 'CLICKED_BUTTON' | 'COPIED_TEXT' | 'COMPLETED_UPLOAD' | 'INSTALLED_APP' There was a problem hiding this comment.
Similarly, we can utilize TS enums here if we want something similarly ergonomic.
https://www.typescriptlang.org/docs/handbook/enums.html#enums-at-compile-time
This example is somewhat of a middle ground between both of your approaches, just a little easier for me to read personally without all the '|' as we add more metrics but also not as verbose as setting the keys and values to the same thing
enum EVENT_METRICS {
VISITED_PAGE,
CLICKED_BUTTON,
COPIED_TEXT,
COMPLETED_UPLOAD,
INSTALLED_APP
}
type EventMetrics = keyof typeof EVENT_METRICS
const mymetric:EventMetrics = 'lol' // This errors because it's looking for the union string type
There was a problem hiding this comment.
Can you add some comments around the enum, to inform people to not actually ever use the enum directly. There are quite a few gotcha's with enums, that we want to avoid at all costs so we don't want people to accidentally use it and cause larger issues.
You can read more about the issues with enums, and I personally think we should avoid them, "Why I Don't Like Enums", but I'm okay with using them as long as we don't export them and just use them to build out the string literal unions
There was a problem hiding this comment.
These are good points, but I think for a change a small as this, how we choose to implement it will have a negligible effect on performance or any other meaningful metric.
I will merge this change as is later today unless we think there is a strong practical reason to go with one approach over the other.
Description
This closes https://github.com/orgs/codecov/projects/31/views/11?pane=issue&itemId=68552864.
We already have some validation server side. This PR adds some TS checking so that we can receive type safe messaging earlier.
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.