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

feat(tanstackstart): Add @sentry/tanstackstart-react package and make @sentry/tanstackstart package a utility package #15629

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

lforst
Copy link
Member

@lforst lforst commented Mar 11, 2025

I realized thanks to TanStack/router#3607 (comment) that TanStack Start is gonna be a generic thing that supports multiple frameworks like React and Solid (maybe even more in the future).

This means that we should probably not publish the package for TanStack Start React as @sentry/tanstackstart. I thought about doing submodule exports like @sentry/tanstackstart/react and @sentry/tanstackstart/solid but that comes with a few (user-facing) drawbacks that I don't want to deal with:

  • We don't know how well submodule exports behave with all the build tooling.
  • Intellisense is going to show stuff from all the different submodule exports which is extremely error-prone and may break apps if you accidentally import the wrong thing, maybe leading to super opaque errors.

For the reasons above I decided to make the @sentry/tanstackstart package a utility package that we can use to host generic instrumentation helpers and publish an additional package called @sentry/tanstackstart-react that contains the actual SDK. For now the utils package is empty but that will change in the future as we see fit.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ke `@sentry/tanstackstart` package a utility package
@lforst lforst requested review from Lms24 and chargome March 11, 2025 10:04
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM! No strong opinions on own package vs. subpath export. I think the reasoning for having @sentry/tastackstart-react it as its own package makes sense, though it's a bit unfortunate that we need more than one package for the framework (related, see my Q about the @sentry/tanstack package).

Other nits (feel free to ignore):

  • let's add the package(s) to the main README in the repo
  • let's update the bug report issue template s.t. users can select the package

@@ -1,7 +1,7 @@
{
"name": "@sentry/tanstackstart",
Copy link
Member

Choose a reason for hiding this comment

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

should this rather be @sentry-internal/tanstackstart then? Or are we planning on instructing users to install both packages?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had thought the same, I opted to do it like this for now because I messed up because I already published the package, so it's out there and grabbing SEO. We definitely need to update the README for the package that's out there for now, so I think this is still fine - maybe I'll deprecate the package and go for the internal name in a future PR. Hope that makes sense. I wish I hadn't published the package already 😂 😭

@chargome
Copy link
Member

Will the framework support using multiple ui libs within one app at some point e.g. like Astro? Then specifically naming the package would cause confusion in the future

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@lforst
Copy link
Member Author

lforst commented Mar 11, 2025

Will the framework support using multiple ui libs within one app at some point e.g. like Astro? Then specifically naming the package would cause confusion in the future

I don't think so. Also we need specific instrumentation APIs, so even if, I think this split would make sense.

@lforst lforst merged commit 9cae1a0 into develop Mar 11, 2025
150 checks passed
@lforst lforst deleted the lforst-tsstart-utility branch March 11, 2025 12:22
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.

None yet

3 participants