feat(react): Add TanStack Router integration #12095

MicahLyle · 2024-05-17T04:41:20Z

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Example Repo I Worked Off Of: https://github.com/MicahLyle/tanstack-router-sentry-exploration

Potential Notes/TODOs/Things to Address

Thank you @lforst for all the help.
I based this mostly off of the Next JS Client Router implementation. It was relatively straightforward because TanStack Router has pretty nice observability with subscribe(...).
It's not clear if const matches = state.pendingMatches ?? state.matches; is the right approach, or if I should bail immediately if pendingMatches is not present. @tannerlinsley any input here?
I took the if (instrumentPageLoad && initPathName) code from the React Router v6 tracing code. It seemed like something we'd want for an SPA: There's only a page load on the initial load and then from there we have only navigation instrumentation. Correct me if I'm wrong though.
@lforst - Handing off tests and docs to you 😅. If we can get an "experimental" release out, I'm happy to help test it in the wild as I have some projects I'll be able to migrate to Tanstack Router in the near future once this is merged.
I "vendored" the types best I could per @lforst's suggestion.
For future improvements, TanStack Router has a lot of other information exposed in the state. I kind of hinted at that by adding some additional type properties that are not currently used, but may end up being useful as this integration is expanded.
I'm just letting Sentry's default mechanics detect when the navigation is done, but I'm presuming we can get that info from TanStack Router itself, so that would probably be a future direction to head in.
There's a number of edge cases and things I didn't check or thoroughly look into, but, again, happy to help test this in the wild once an initial version is merged.

lforst · 2024-05-21T09:05:06Z

Thanks again for doing this! I'll need some time to catch up on things with our v8 release but I'll make sure that this makes it in!

AbhiPrasad · 2024-05-21T18:28:49Z

packages/react/src/tanstackrouter.ts

+			const initPathName = WINDOW && WINDOW.location && WINDOW.location.pathname;
+			if (instrumentPageLoad && initPathName) {
+				startBrowserTracingPageLoadSpan(client, {
+					name: initPathName,


is there no way for us to get the routeId for the pageload? We can read this from router.state also right?

Yes, I believe so. Sorry, I missed putting it there. @lforst can you put that in?

Route ID is on each match. You'll probably want the deepest one: matches[matches.length - 1].routeId

Entire route config is available at: router.routesById[routeId]

I did the matches[matches.length - 1].routeId approach below, just forgot to put it here as well.

ruiaraujo012 · 2024-05-23T08:41:25Z

Hi @MicahLyle, this thread on Tanstack discord could help you here https://discord.com/channels/719702312431386674/1240064824021483570

TkDodo · 2024-05-23T08:56:40Z

packages/react/src/tanstackrouter.ts

+export function tanstackRouterInstrumentNavigation(router: TanstackRouter, client: Client): void {
+	router.history.subscribe(() => {
+		const state = router.state;
+		const matches = state.pendingMatches ?? state.matches;


it seems like what we do internally is more like this:

const matches = [ ...state.cachedMatches, ...(state.pendingMatches ?? []), ...state.matches, ]

see: https://github.com/TanStack/router/blob/bce8e71652acdfa66594fedaab4bc5f058ccaf6e/packages/react-router/src/RouterProvider.tsx#L113-L117

I did find multiple implementations of this with different spreading order, so I'm not sure which one would be correct 😅

You'll want one of these depending on the situation:

state.pendingMatches || state.matches - The user may be in the process of navigating to a new location, which is why you would want pendingMatches to represent that pending navigation

state.matches - If you're only interested in where the user is currently and are not interested in any potential pending navigation destinations, then this should suffice.

I recommend the former. Do not use state.cachedMatches for anything outside of reporting back stuff that's cached.

@tannerlinsley I did const matches = state.pendingMatches ?? state.matches;. Would that be worse than state.pendingMatches || state.matches?

TkDodo · 2024-05-23T08:56:54Z

packages/react/src/tanstackrouter.ts

+}
+
+export function tanstackRouterInstrumentNavigation(router: TanstackRouter, client: Client): void {
+	router.history.subscribe(() => {


not sure about the scope, but do we not need to unsubscribe at one point?

Yeah we can add one, we just need to hook it onto closing the Sentry client, which we currently don't have an exposed hook for in the public API.

lforst · 2024-05-28T14:58:49Z

Ok, I got this into a minimal working state and added some tests. Not 100% sure on the implementation but I think it should work correctly.

One problem I was facing is that we couldn't just straight up use the router.history.subscribe() hook to trigger navigations because at the time it fires, the state is stale and pendingMatches is still undefined. The things we need (ie. the new parameterized route) aren't inside the state yet so what we do is we take the pathname as the default navigation name and update it with proper values as soon as the onResolved hook fires. Since the onResolved hook doesn't give us the parameterized name, we throw the resolved pathname and search values into router.matchRoutes() to get the parameterized route.

I also dumped the route params onto the span as attributes.

@AbhiPrasad would appreciate another pass!

AbhiPrasad · 2024-05-29T13:26:37Z

packages/react/src/tanstackrouter.ts

+
+// The following types are vendored types from TanStack Router, so we don't have to depend on the actual package
+
+export interface VendoredTanstackRouter {


let's move this into a separate file and copy over license as well, I think we need to give attribution.

AbhiPrasad · 2024-05-29T13:30:53Z

packages/react/src/tanstackrouter.ts

+
+  const paramAttributes: Record<string, string> = {};
+  for (const key of Object.keys(match.params)) {
+    paramAttributes[`params.${key}`] = match.params[key];


should we only set this with sendDefaultPii?

Also, I think we should call this url.path.params.X because the OTEL attribute for path is url.path.

should we only set this with sendDefaultPii?

I would opt against it since all of the params are inside the URL (which we send by default) anyhow.

If for some reason, we would be sending additional data after a patch, I would guard it behind sendDefaultPii.

Also, I think we should call this url.path.params.X because the OTEL attribute for path is url.path

👍

mydea · 2024-05-29T13:33:21Z

dev-packages/e2e-tests/test-applications/tanstack-router/package.json

+    "@vitejs/plugin-react-swc": "^3.5.0",
+    "typescript": "^5.2.2",
+    "vite": "^5.2.0",
+    "@playwright/test": "^1.41.1"


Suggested change

"@playwright/test": "^1.41.1"

"@playwright/test": "^1.41.1",

"@sentry-internal/event-proxy-server": "link:../../../event-proxy-server"

I think this is missing here...?

It's odd but it doesn't seem to matter. I think because the package is available in the root node_modules. Still gonna add it.

yeah, I think that's why it works, we should still add it for safety/correctness I guess :D

Initial TanStack Router Integration

7cd66ce

AbhiPrasad requested review from lforst and AbhiPrasad May 21, 2024 18:24

AbhiPrasad reviewed May 21, 2024

View reviewed changes

AbhiPrasad mentioned this pull request May 21, 2024

Support Tanstack Router for performance instrumentation #7927

Closed

TkDodo reviewed May 23, 2024

View reviewed changes

lforst self-assigned this May 27, 2024

lforst changed the title ~~Initial TanStack Router Integration~~ feat(react): Add TanStack Router integration May 27, 2024

lforst added 6 commits May 28, 2024 09:40

Clean things up a bit

2983c42

Add E2E tests

7c839ed

fix

af12b07

Merge branch 'develop' into develop

a3a2008

Add playwright dep

fbf0247

There goes an hour I'll never get back

a27f2da

lforst requested review from AbhiPrasad, tannerlinsley and TkDodo May 28, 2024 14:59

lforst added 2 commits May 29, 2024 08:23

Use new onBeforeNavigate hook

17242cc

Merge branch 'develop' into develop

2e9ca18

AbhiPrasad reviewed May 29, 2024

View reviewed changes

mydea reviewed May 29, 2024

View reviewed changes

mydea approved these changes May 29, 2024

View reviewed changes

Implement review feedback

8a5b592

lforst merged commit 0d1093d into getsentry:develop May 29, 2024
66 checks passed

lforst mentioned this pull request May 29, 2024

Add TanStack Router docs getsentry/sentry-docs#10193

Merged

6 tasks

data-sync-user mentioned this pull request Jun 3, 2024

Bump @sentry/browser from 7.113.0 to 8.7.0 MozillaFoundation/foundation.mozilla.org#12409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(react): Add TanStack Router integration #12095

feat(react): Add TanStack Router integration #12095

MicahLyle commented May 17, 2024 •

edited

lforst commented May 21, 2024

AbhiPrasad May 21, 2024

MicahLyle May 21, 2024

tannerlinsley May 23, 2024

MicahLyle May 23, 2024

ruiaraujo012 commented May 23, 2024 •

edited

TkDodo May 23, 2024

tannerlinsley May 23, 2024

MicahLyle May 23, 2024

TkDodo May 23, 2024

AbhiPrasad May 23, 2024

lforst commented May 28, 2024 •

edited

AbhiPrasad May 29, 2024

AbhiPrasad May 29, 2024

lforst May 29, 2024

mydea May 29, 2024

lforst May 29, 2024 •

edited

mydea May 29, 2024


		// The following types are vendored types from TanStack Router, so we don't have to depend on the actual package

		export interface VendoredTanstackRouter {

	"@playwright/test": "^1.41.1"
	"@playwright/test": "^1.41.1",
	"@sentry-internal/event-proxy-server": "link:../../../event-proxy-server"

feat(react): Add TanStack Router integration #12095

feat(react): Add TanStack Router integration #12095

Conversation

MicahLyle commented May 17, 2024 • edited

lforst commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruiaraujo012 commented May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lforst commented May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lforst May 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicahLyle commented May 17, 2024 •

edited

ruiaraujo012 commented May 23, 2024 •

edited

lforst commented May 28, 2024 •

edited

lforst May 29, 2024 •

edited