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

ref(solidstart): Simplify middleware structure without subexport #13494

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

andreiborza
Copy link
Member

⚠️ This is a breaking change ⚠️

Simplifies the package by getting rid of the @sentry/solidstart/middleware subexport in favor of a regular export.

The middleware now needs to be imported like this:

import { sentryBeforeResponseMiddleware } from '@sentry/solidstart';

@andreiborza andreiborza added the Package: solidstart Issues related to the Sentry SolidStart SDK label Aug 28, 2024
Copy link
Contributor

github-actions bot commented Aug 28, 2024

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 22.52 KB - -
@sentry/browser (incl. Tracing) 34.85 KB - -
@sentry/browser (incl. Tracing, Replay) 71.21 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 64.48 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 75.57 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 88.29 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.13 KB - -
@sentry/browser (incl. metrics) 26.83 KB - -
@sentry/browser (incl. Feedback) 39.59 KB - -
@sentry/browser (incl. sendFeedback) 27.18 KB - -
@sentry/browser (incl. FeedbackAsync) 31.9 KB - -
@sentry/react 25.28 KB - -
@sentry/react (incl. Tracing) 37.83 KB - -
@sentry/vue 26.66 KB - -
@sentry/vue (incl. Tracing) 36.68 KB - -
@sentry/svelte 22.65 KB - -
CDN Bundle 23.77 KB - -
CDN Bundle (incl. Tracing) 36.53 KB - -
CDN Bundle (incl. Tracing, Replay) 70.86 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.16 KB - -
CDN Bundle - uncompressed 69.61 KB - -
CDN Bundle (incl. Tracing) - uncompressed 108.28 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 219.66 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.85 KB - -
@sentry/nextjs (client) 37.6 KB - -
@sentry/sveltekit (client) 35.44 KB - -
@sentry/node 115.71 KB - -
@sentry/node - without tracing 90.1 KB - -
@sentry/aws-serverless 99.51 KB - -

View base workflow run

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.

I have a basic question: What was the reason we exported it as a sub-export previously and why do we not need to do it anymore? Also is there a concrete reason not to export the middleware as a sub-export?

I'm asking because I wanna make sure we're not accidentally importing something from a solid package in the middleware which has the potential to break otel instrumentation (we've had this in NestJS).

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.

Discussed my previous comment offline, LGTM!

(For future readesrs: Main that the middleware doesn't have to be a sub-export as well as discoverability for users and simplification of build process)

@andreiborza
Copy link
Member Author

I have a basic question: What was the reason we exported it as a sub-export previously and why do we not need to do it anymore? Also is there a concrete reason not to export the middleware as a sub-export?

I'm asking because I wanna make sure we're not accidentally importing something from a solid package in the middleware which has the potential to break otel instrumentation (we've had this in NestJS).

As discussed, the subexport pattern has been first used for the solidrouter integration so that we can mark solid router as an optional dependency.

For the middleware, I previously thought we might want to keep it out of the main package but there really is no benefit here, it's simpler for users and explanation to just import this straight from the root.

@andreiborza andreiborza merged commit 2b3e26f into develop Aug 28, 2024
99 checks passed
@andreiborza andreiborza deleted the ab/solidstart-middleware-remove-subexport branch August 28, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: solidstart Issues related to the Sentry SolidStart SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants