chore(telemetry): refactor telemetry types VSCODE-672 #912
Conversation
nirinchev
requested review from
alenakhineika and
gagik
and removed request for
gagik
nirinchev
changed the title
gagik
left a comment
gagik
left a comment
There was a problem hiding this comment.
This is great; previous way of adding a new event for telemetry required a lot of boilerplate so this is a really nice change
src/telemetry/index.ts
Outdated
| export * from './telemetryEvents'; | ||
|
|
||
| export { TelemetryService }; | ||
| export default TelemetryService; |
There was a problem hiding this comment.
this is a more controversial preference thing but can we consider not using default exports (and maybe even banning them eventually...)? I know TelemetryService was actually mainly being used as a default export and we have a lot in the codebase.
I think benefits of avoiding defaults are quite minor especially since IDEs have gotten better but minor boosts still exist (also consistency as I don't really see why sometimes we have default and sometimes named exports). Strongest argument other than that might be:
Default exports are also refactoring hostile. If I rename the named exported members in a module, then any usage will be renamed, too (provided you are using an IDE that can apply this refactoring). With default exports, this isn’t possible.
https://www.lloydatkinson.net/posts/2022/default-exports-in-javascript-modules-are-terrible/
https://cichocinski.dev/blog/using-default-exports-makes-javascript-harder
There was a problem hiding this comment.
I'd not be opposed to that - the only reason I reexported it as default was to avoid changing a bunch of places where we were importing it from telemetry/telemetryService.
There was a problem hiding this comment.
Yeah that makes sense, it may be worth leaving for a different time and dealing with default exports more globally later if we decide to do so
Description
This is a refactoring of the telemetry service to more closely tie the event type with its properties.
Keytar Secrets Migration Failedas it wasn't ever reported.There should be no functional changes and all events should continue to be reported exactly as before. This is laying the groundwork for the telemetry changes we've been discussing, but those are not included in this PR.
Motivation and Context