Skip to content

Commit 8874ad0

Browse files
authored
ref: move use-button-tracking from sentry hooks to react context (#95220)
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?
1 parent 7223c04 commit 8874ad0

File tree

14 files changed

+138
-129
lines changed

14 files changed

+138
-129
lines changed

knip.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const productionEntryPoints = [
99
// dynamic imports _not_ recognized by knip
1010
'static/app/bootstrap/{index,initializeMain}.tsx',
1111
'static/gsApp/initializeBundleMetrics.tsx',
12+
'static/gsApp/registerHooks.tsx',
1213
// defined in webpack.config pipelines
1314
'static/app/utils/statics-setup.tsx',
1415
'static/app/views/integrationPipeline/index.tsx',

static/app/bootstrap/initializeApp.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,17 @@ import {processInitQueue} from './processInitQueue';
1010
import {renderMain} from './renderMain';
1111
import {renderOnDomReady} from './renderOnDomReady';
1212

13-
export function initializeApp(config: Config) {
13+
export function initializeApp(
14+
config: Config,
15+
SentryHooksProvider?: React.ComponentType<React.PropsWithChildren>
16+
) {
1417
initializeSdk(config);
1518
// Initialize the config store after the SDK, so we can log errors to Sentry during config initialization if needed. N.B. This mutates the config slightly
1619
commonInitialization(config);
1720

1821
// Used for operational metrics to determine that the application js
1922
// bundle was loaded by browser.
2023
metric.mark({name: 'sentry-app-init'});
21-
renderOnDomReady(renderMain);
24+
renderOnDomReady(renderMain(SentryHooksProvider));
2225
processInitQueue();
2326
}

static/app/bootstrap/initializeMain.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@ import type {Config} from 'sentry/types/system';
22

33
import {initializeLocale} from './initializeLocale';
44

5-
export async function initializeMain(config: Config) {
5+
export async function initializeMain(
6+
config: Config,
7+
SentryHooksProvider?: React.ComponentType<React.PropsWithChildren>
8+
) {
69
// This needs to be loaded as early as possible, or else the locale library can
710
// throw an exception and prevent the application from being loaded.
811
//
@@ -12,5 +15,5 @@ export async function initializeMain(config: Config) {
1215
// This is dynamically imported because we need to make sure locale is configured
1316
// before proceeding.
1417
const {initializeApp} = await import('./initializeApp');
15-
initializeApp(config);
18+
initializeApp(config, SentryHooksProvider);
1619
}

static/app/bootstrap/renderMain.tsx

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,26 @@
1+
import type * as React from 'react';
2+
13
import {ROOT_ELEMENT} from 'sentry/constants';
24
import Main from 'sentry/main';
35

46
import {renderDom} from './renderDom';
57

6-
export function renderMain() {
7-
try {
8-
renderDom(Main, `#${ROOT_ELEMENT}`, {});
9-
} catch (err) {
10-
if (err.message === 'URI malformed') {
11-
// eslint-disable-next-line no-console
12-
console.error(
13-
new Error(
14-
'An unencoded "%" has appeared, it is super effective! (See https://github.com/ReactTraining/history/issues/505)'
15-
)
16-
);
17-
window.location.assign(window.location.pathname);
8+
export function renderMain(
9+
SentryHooksProvider?: React.ComponentType<React.PropsWithChildren>
10+
) {
11+
return () => {
12+
try {
13+
renderDom(Main, `#${ROOT_ELEMENT}`, {SentryHooksProvider});
14+
} catch (err) {
15+
if (err.message === 'URI malformed') {
16+
// eslint-disable-next-line no-console
17+
console.error(
18+
new Error(
19+
'An unencoded "%" has appeared, it is super effective! (See https://github.com/ReactTraining/history/issues/505)'
20+
)
21+
);
22+
window.location.assign(window.location.pathname);
23+
}
1824
}
19-
}
25+
};
2026
}

static/app/components/core/button/useButtonFunctionality.tsx

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,7 @@ export function useButtonFunctionality(props: ButtonProps | LinkButtonProps) {
1313
props['aria-label'] ??
1414
(typeof props.children === 'string' ? props.children : undefined);
1515

16-
const buttonTracking = useButtonTracking({
17-
analyticsEventName: props.analyticsEventName,
18-
analyticsEventKey: props.analyticsEventKey,
19-
analyticsParams: {
20-
priority: props.priority,
21-
href: 'href' in props ? props.href : undefined,
22-
...props.analyticsParams,
23-
},
24-
'aria-label': accessibleLabel || '',
25-
});
16+
const buttonTracking = useButtonTracking();
2617

2718
const handleClick = (e: React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>) => {
2819
// Don't allow clicks when disabled or busy
@@ -32,7 +23,16 @@ export function useButtonFunctionality(props: ButtonProps | LinkButtonProps) {
3223
return;
3324
}
3425

35-
buttonTracking();
26+
buttonTracking({
27+
analyticsEventName: props.analyticsEventName,
28+
analyticsEventKey: props.analyticsEventKey,
29+
analyticsParams: {
30+
priority: props.priority,
31+
href: 'href' in props ? props.href : undefined,
32+
...props.analyticsParams,
33+
},
34+
'aria-label': accessibleLabel || '',
35+
});
3636
// @ts-expect-error at this point, we don't know if the button is a button or a link
3737
props.onClick?.(e);
3838
};

static/app/components/core/trackingContext.tsx

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ import {createContext, useContext} from 'react';
22

33
import type {DO_NOT_USE_ButtonProps as ButtonProps} from './button/types';
44

5-
const defaultButtonTracking = (props: ButtonProps) => {
5+
const defaultButtonTracking = () => {
66
const hasAnalyticsDebug = window.localStorage?.getItem('DEBUG_ANALYTICS') === '1';
7-
return () => {
7+
return (props: ButtonProps) => {
88
const hasCustomAnalytics =
99
props.analyticsEventName || props.analyticsEventKey || props.analyticsParams;
1010
if (hasCustomAnalytics && hasAnalyticsDebug) {
@@ -21,13 +21,13 @@ const defaultButtonTracking = (props: ButtonProps) => {
2121
};
2222

2323
const TrackingContext = createContext<{
24-
useButtonTracking?: (props: ButtonProps) => () => void;
24+
buttonTracking?: (props: ButtonProps) => void;
2525
}>({});
2626

2727
export const TrackingContextProvider = TrackingContext.Provider;
2828

29-
export const useButtonTracking = (props: ButtonProps) => {
29+
export const useButtonTracking = () => {
3030
const context = useContext(TrackingContext);
3131

32-
return context.useButtonTracking?.(props) ?? defaultButtonTracking(props);
32+
return context.buttonTracking ?? defaultButtonTracking();
3333
};

static/app/index.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,11 @@ async function app() {
9191
// getsentry augments Sentry's application through a 'hook' mechanism. Sentry
9292
// provides various hooks into parts of its application. Thus all getsentry
9393
// functionality is initialized by registering its hook functions.
94-
const {default: registerHooks} = await registerHooksImport;
94+
const {default: registerHooks, SentryHooksProvider} = await registerHooksImport;
9595
registerHooks();
9696

9797
const {initializeMain} = await initalizeMainImport;
98-
initializeMain(config);
98+
initializeMain(config, SentryHooksProvider);
9999

100100
const {initializeBundleMetrics} = await initalizeBundleMetricsImport;
101101
initializeBundleMetrics();

static/app/main.tsx

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,29 +8,30 @@ import {OnboardingContextProvider} from 'sentry/components/onboarding/onboarding
88
import {ThemeAndStyleProvider} from 'sentry/components/themeAndStyleProvider';
99
import {USE_REACT_QUERY_DEVTOOL} from 'sentry/constants';
1010
import {routes} from 'sentry/routes';
11-
import {SentryTrackingProvider} from 'sentry/tracking';
1211
import {DANGEROUS_SET_REACT_ROUTER_6_HISTORY} from 'sentry/utils/browserHistory';
1312

1413
import {buildReactRouter6Routes} from './utils/reactRouter6Compat/router';
1514

16-
function buildRouter() {
15+
function buildRouter(SentryHooksProvider?: React.ComponentType<React.PropsWithChildren>) {
1716
const sentryCreateBrowserRouter = wrapCreateBrowserRouterV6(createBrowserRouter);
18-
const router = sentryCreateBrowserRouter(buildReactRouter6Routes(routes()));
17+
const router = sentryCreateBrowserRouter(
18+
buildReactRouter6Routes(routes(SentryHooksProvider))
19+
);
1920
DANGEROUS_SET_REACT_ROUTER_6_HISTORY(router);
2021

2122
return router;
2223
}
2324

24-
function Main() {
25-
const [router] = useState(buildRouter);
25+
function Main(props: {
26+
SentryHooksProvider?: React.ComponentType<React.PropsWithChildren>;
27+
}) {
28+
const [router] = useState(() => buildRouter(props.SentryHooksProvider));
2629

2730
return (
2831
<AppQueryClientProvider>
2932
<ThemeAndStyleProvider>
3033
<OnboardingContextProvider>
31-
<SentryTrackingProvider>
32-
<RouterProvider router={router} />
33-
</SentryTrackingProvider>
34+
<RouterProvider router={router} />
3435
</OnboardingContextProvider>
3536
{USE_REACT_QUERY_DEVTOOL && (
3637
<ReactQueryDevtools initialIsOpen={false} buttonPosition="bottom-left" />

static/app/routes.tsx

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ import {makeLazyloadComponent as make} from './makeLazyloadComponent';
4242

4343
const hook = (name: HookName) => HookStore.get(name).map(cb => cb());
4444

45-
function buildRoutes() {
45+
function buildRoutes(
46+
SentryHooksProvider: React.ComponentType<React.PropsWithChildren> = Fragment
47+
) {
4648
// Read this to understand where to add new routes, how / why the routing
4749
// tree is structured the way it is, and how the lazy-loading /
4850
// code-splitting works for pages.
@@ -2560,18 +2562,24 @@ function buildRoutes() {
25602562
);
25612563

25622564
const appRoutes = (
2563-
<ProvideAriaRouter>
2564-
<Route>
2565-
{experimentalSpaRoutes}
2566-
<Route path="/" component={errorHandler(App)}>
2567-
{rootRoutes}
2568-
{authV2Routes}
2569-
{organizationRoutes}
2570-
{legacyRedirectRoutes}
2571-
<Route path="*" component={errorHandler(RouteNotFound)} />
2572-
</Route>
2565+
<Route
2566+
component={({children}: {children: React.ReactNode}) => {
2567+
return (
2568+
<ProvideAriaRouter>
2569+
<SentryHooksProvider>{children}</SentryHooksProvider>
2570+
</ProvideAriaRouter>
2571+
);
2572+
}}
2573+
>
2574+
{experimentalSpaRoutes}
2575+
<Route path="/" component={errorHandler(App)}>
2576+
{rootRoutes}
2577+
{authV2Routes}
2578+
{organizationRoutes}
2579+
{legacyRedirectRoutes}
2580+
<Route path="*" component={errorHandler(RouteNotFound)} />
25732581
</Route>
2574-
</ProvideAriaRouter>
2582+
</Route>
25752583
);
25762584

25772585
return appRoutes;

static/app/tracking.tsx

Lines changed: 0 additions & 15 deletions
This file was deleted.

0 commit comments

Comments
 (0)