sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

Initial TanStack Router Integration

Open MicahLyle opened this issue 9 months ago • 2 comments

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.

MicahLyle avatar May 17 '24 04:05 MicahLyle

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!

lforst avatar May 21 '24 09:05 lforst

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

ruiaraujo012 avatar May 23 '24 08:05 ruiaraujo012

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!

lforst avatar May 28 '24 14:05 lforst