Skip to content

feat(react-start-api-routes): Write the matched route path as unenumerable to request object #3607

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

Conversation

lforst
Copy link

@lforst lforst commented Feb 28, 2025

This comes a bit out of nowhere but I am currently building the official Sentry SDK for TSStart and one thing that is really important for tracing/spans/opentelemetry is being able to group stuff by the parameterized route.

I haven't found a nice way in the public API yet to allow reading the parameterized route in a central location that doesn't require users to wrap every single route they create.

This is probably a bit of a hack but it seemed unintrusive enough for me (since it is unenumerable) and is getting the job done wonderfully.

@birkskyum
Copy link
Member

@lforst , just an FYI there will likely be tanstack start for multiple frameworks, in case the sentry sdk is react-specific you might want to add react- somehwere in the package name. See i.e. early work here on a solid version.

@schiller-manuel
Copy link
Contributor

for API route I hope we have a single package in the future.

w.r.t. to the sentry integration: I think it would make sense to offer a generic customization point where sentry could hook in. then this request tag would solely live in e.g. sentry

@lforst
Copy link
Author

lforst commented Mar 3, 2025

@lforst , just an FYI there will likely be tanstack start for multiple frameworks, in case the sentry sdk is react-specific you might want to add react- somehwere in the package name. See i.e. early work here on a solid version.

That's a great point. I somehow missed that tss will be generic. I'll publish a package with an updated name. Thanks!

@lforst
Copy link
Author

lforst commented Mar 3, 2025

for API route I hope we have a single package in the future.

That's dope 👌

w.r.t. to the sentry integration: I think it would make sense to offer a generic customization point where sentry could hook in. then this request tag would solely live in e.g. sentry

Just gauging interest in this change in general, would it make sense to add this interim until there is a proper public API? I would be like 0% mad if you all decide to break this change when you switch over to a generic customization.

I am also happy to add a hook somewhere if you give me some guidance on how you would want it to look like 😄 This is kinda super duper mission critical for Sentry so I am happy to sacrifice some time on this.

@birkskyum
Copy link
Member

for API route I hope we have a single package in the future.

That's dope 👌

@lforst the future is now.

https://npmjs.com/@tanstack/start-api-routes

@lforst
Copy link
Author

lforst commented Mar 11, 2025

Noice. How shall we proceed here?

@birkskyum
Copy link
Member

This PR would have to be rebased, so the change is applied instead to the shared start-api-routes package

@lforst
Copy link
Author

lforst commented Mar 11, 2025

This PR would have to be rebased, so the change is applied instead to the shared start-api-routes package

rebased 👍

Copy link

nx-cloud bot commented Mar 11, 2025

View your CI Pipeline Execution ↗ for commit 43ad644.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 1m View ↗

☁️ Nx Cloud last updated this comment at 2025-03-11 10:38:18 UTC

@schiller-manuel
Copy link
Contributor

we'll be adding middleware support for API routes which should cover this use case

@lforst
Copy link
Author

lforst commented Mar 11, 2025

we'll be adding middleware support for API routes which should cover this use case

Is there a timeline for that?

@lforst
Copy link
Author

lforst commented Mar 11, 2025

Discussed on Discord w/ @schiller-manuel that this change will likely become obsolete with global api middlewares.

@lforst lforst closed this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants