Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(react): Match routes with parseSearch option in TanStack Router instrumentation #14328

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

lsmurray
Copy link
Contributor

The current sentry implementation was breaking in my application because we use custom search param serialization.

The default tanstack implementation uses this.options.parseSearch to parse the search string into a parsed location

https://github.com/TanStack/router/blob/55891518fcfa7c308e1db4149d357cf38700c882/packages/react-router/src/router.ts#L1015-L1016

The matchRoutes signature that sentry was using has been deprecated.

https://github.com/TanStack/router/blob/55891518fcfa7c308e1db4149d357cf38700c882/packages/react-router/src/router.ts#L1063-L1098

The signature only exists to support sentry as far as I can tell

TanStack/router#2164
TanStack/router#2165

This PR implements two fixes

  1. use the new matchRoutes signature instead of the deprecated signature
  2. use parseSearch to support applications that use custom search param serialization.

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).

@lsmurray lsmurray force-pushed the luke/fix-tanstack-match-routes branch from 5ba4f26 to 67bb292 Compare November 16, 2024 21:28
@lsmurray
Copy link
Contributor Author

@lforst @mydea I checked the git blame and saw the two of you worked on the implementation of this feature. Wanted to politely bring this to your attention.

@lforst
Copy link
Member

lforst commented Nov 21, 2024

@lsmurray Unfortunately this is (rightfully) failing the tests. Using the new signature would be a breaking change because it would break on TanStack versions we already promised to support. Do you think it would be possible to isolate your PR to only the parseSearch change?

@lsmurray
Copy link
Contributor Author

good catch - will isolate it

@lsmurray
Copy link
Contributor Author

I've updated the PR - the only location we have to parse search is initial load since we're getting search from the window. In other places where we get search from the location the search has already been parsed.

@lsmurray
Copy link
Contributor Author

lsmurray commented Dec 3, 2024

@lforst - let me know if there is anything else you need from me to get this across the finish line.

Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops sorry I fumbled this PR after running the tests. Lgtm - thanks for the reminder!

@lforst lforst changed the title fix(react): Tanstack Router integration matchRoutes signature changed fix(react): Match routes with parseSearch option in TanStack Router instrumentation Dec 3, 2024
@lforst lforst merged commit b0729b6 into getsentry:develop Dec 3, 2024
135 checks passed
mydea pushed a commit that referenced this pull request Dec 3, 2024
This PR adds the external contributor to the CHANGELOG.md file, so that
they are credited for their contribution. See #14328

Co-authored-by: lforst <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants