ref: decouple button from HookStore #93940
Conversation
github-actions
bot
added
the
Scope: Frontend
static/app/main.tsx
Outdated
|
|
||
| return ( | ||
| <AppQueryClientProvider> | ||
| <ThemeAndStyleProvider> | ||
| <OnboardingContextProvider> | ||
| <RouterProvider router={router} /> | ||
| <TrackingContextProvider value={trackingContextValue}> |
There was a problem hiding this comment.
Is there any other place where we would need to put this Provider?
For tests, it doesn’t matter as we can just fallback to the default implementation...
There was a problem hiding this comment.
I could also extract this to a SentryTrackingProvider that encapsulates the logic if we don’t want to pollute the Main component.
There was a problem hiding this comment.
This is a step in the right direction! Just curious if we've considered something like a global click listener that reads from data- attributes? IMO that would be cleaner and allow us to completely separate the design system component from the specific implementation needs of the Sentry app.
haven’t thought about this. I don’t think this would work if buttons call and then there’s |
) To be able to decouple our `core/components` from getsentry specific logic, we have to stop using sentry-hooks inside `core/components`. However, button tracking logic is defined in getentry and injected via `'react-hook:use-button-tracking'`, which tracks button click events in sentry itself. The react idiomatic way to inject specific logic into a “generic” library is “dependency injection” via react context provided by the generic library. In our case, that means `core/components/button` exposes a `<TrackingContextProvider>` that consumers can render and pass a function that will be called when a button is clicked. This is similar to how react-aria allows link behaviour to be injected via their `RouterProvider`. This is what I’ve been doing in the base branch: - #93940 Now here’s the thing: sentry hooks do pretty much the same thing for us as react context - at least for things that we only use within react. But that means we’ve now created another layer of indirection. Basically, the flow is: ``` HookStore --> React Context --> Button ``` the sentry specific `SentryTrackingProvider` basically takes the value we’ve put into the hook-store and forwards it to the agnostic `TrackingContextProvider` coming from our core components: ``` export function SentryTrackingProvider({children}: {children: React.ReactNode}) { const useButtonTracking = HookStore.get('react-hook:use-button-tracking')[0]; const trackingContextValue = useMemo(() => ({useButtonTracking}), [useButtonTracking]); return ( <TrackingContextProvider value={trackingContextValue}> {children} </TrackingContextProvider> ); } ``` This layer of indirection feels unnecessary, and passing hooks around as values is technically against the rules of react (see [never pass hooks around as regular values](https://react.dev/reference/rules/react-calls-components-and-hooks#never-pass-around-hooks-as-regular-values)). --- In this PR, I’m trying to remove that indirection and just do: ``` React Context --> Button ``` At the top level, where we `import('getsentry/registerHooks')`, we now not only get back `registerHooks`, but also a `SentryHooksProvider` that directly renders sentry+react specific things. Right now, it’s just the `TrackingContext`, but it could be other things, too: ``` export function SentryHooksProvider({children}: {children?: React.ReactNode}) { const buttonTracking = useButtonTracking(); const trackingContextValue = useMemo(() => ({buttonTracking}), [buttonTracking]); return ( <TrackingContextProvider value={trackingContextValue}> {children} </TrackingContextProvider> ); } ``` We then take this Provider and have to drill it down a bunch of levels (this is the main drawback tbh), and render it as a top-level provider in our root route. This is necessary to be able to call hooks like `useRouter()` inside the custom hooks. With this, we’d have: - one layer to inject functionality into react instead of two. - using a standardized system (react context) instead of our own abstraction. - calling the `useButtonTracking` hook directly instead of passing it around as a value - removed the `react-hook:use-button-tracking` sentry hook If we agree to this approach, I’d like to follow up with removing all sentry hooks starting with `react-hook:`, and potentially even more sentry hooks that are only used inside react. Please let me know if I’m missing something, e.g. if those hooks are also used by self-hosted customers to show their own things?
static/app/bootstrap/renderMain.tsx
Outdated
| @@ -1,20 +1,26 @@ | |||
| import type * as React from 'react'; | |||
There was a problem hiding this comment.
we technically have react as a global already. don't need the import
There was a problem hiding this comment.
removed here: e538ea6
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #93940 +/- ##
==========================================
- Coverage 80.21% 77.82% -2.40%
==========================================
Files 10538 10538
Lines 607499 607473 -26
Branches 23803 23799 -4
==========================================
- Hits 487331 472758 -14573
- Misses 119782 134330 +14548
+ Partials 386 385 -1 |
This PR decouples
core/buttonfrom the sentry specificHookStoreby using another layer of indirection via React Context. The goal here is to make surecorecomponents are independent.Ideally, I would like to remove the
HookStorevaluereact-hook:use-button-trackingbecause, as far as I can see, we always do the following now:HookStorewill returnundefined, puts that in context, the button reads it and falls back to the default tracking impl.getSentry, we register the hooks, read those, put that into context and use those.So ideally, it would just be:
This would not only get rid of the extra layer of indirection, it would also allow us to just put a function
(props) => voidinto context rather than the extra indirection(props) => () => void, and we wouldn’t need to pass a real react hook around. Passing hooks as values is discouraged as the react compiler won’t be able to optimize those (it’s also against the rules of react, strictly speaking).The problem is that registering hooks happens very early, outside of a react context, and then we just render the app.
W would probably need to refactor this towards:
where
Wrapperwould render the correct context providers around the app that gets rendered inrenderMain.Unless I’m missing something, this means we could get rid of the
HookStorealtogether, at least for things we don’t need outside of react? Please tell me what I’m missing 😅@evanpurkhiser @JonasBa FYI