feat(nav): add analytics tracking #80512
Conversation
github-actions
bot
added
the
Scope: Frontend
| /** | ||
| * A unique identifier string, used as a key for analytics | ||
| */ | ||
| id: string; |
There was a problem hiding this comment.
If there a reasonable chance (and from experience, I would probably say that there is), then I would rather namespace this under analytics object. It's not much of a difference, but it gives you more flexibility and isolation of the analytics and item concept (you'd just access it via item.analytics.id)
There was a problem hiding this comment.
On a separate note, you could apply inversion of control here, and define this as a callback function, so that if someone needs to extend anything, they don't need to go through the item definition and add fields, bloating the struct
| function SidebarLink({ | ||
| children, | ||
| item, | ||
| onClick, | ||
| }: SidebarItemProps & { | ||
| children: React.ReactNode; | ||
| onClick: React.HTMLAttributes<HTMLAnchorElement>['onClick']; | ||
| }) { |
There was a problem hiding this comment.
The type intersection here is a big no-no :D
I recommend pretty much everyone to read the typescript performance docs to understand exactly why.
In summary, TS is unfortunately not very good at intersecting types - the larger the type, the more the compiles struggles (afaik there is also no type caching here, so the more instances you have, the slower things get). These HTMLAttributes types, while seemingly harmless, are actually absolute massive, as they contain all enumerable html attributes, including the entire CSS definition.
A bit of historical context, but about 3 years back when we joined Sentry, things were so bad that making a simple type change took the lsp more than a second to process... You would make a change and it literally took one second or more for the changes to reflect in your IDE.
We basically halved the time and iirc memory usage of the type checker, mostly by removing intersections like these. There is a PR where I did the initial exploration, and then if you search for ref(tsc), there is a bunch of PRs like this one where you can see the impact of these changes.
Adds
trackAnalyticscall for primary nav items on the new sidebar