-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
build: Use Loader Script instead of @sentry/gatsby
#6885
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hmm, is there a way to test this with Sentry enabled in a vercel preview? 🤔 |
| "@mdx-js/react": "^1.6.18", | ||
| "@sentry-internal/global-search": "^0.5.7", | ||
| "@sentry/gatsby": "^7.46.0", | ||
| "@sentry/browser": "7.51.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for types, as we access Sentry.getCurrentHub() in few places. Not sure, we can also get rid of this and just tread these as any 🤔 WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah the types :/
feels fine to include this then, since we won't break the hub top level api.
another thing about using the loader that is kind of a pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jup, true! Maybe if we want to push the loader further, we can think about having a @sentry/loader package that provides some shims on top of window + types - also, as in this PR, if you use the loader and want to access some APIs in typed/module code, it's a bit of a pain as you need to do a lot of guarding etc. manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the loader package is a good idea, because this will be confusing (folks might open PR bumping via yarn)
This replaces the usage of
@sentry/gatbsywith usage of the Loader Script.This is done so we use the Loader Script in an actual application, allowing us to verify this. The dedicated gatsby package does not add a lot on top of what the loader offers, and we still dogfood it in the develop docs.
Note that I noticed that we have already been providing a custom webpack plugin config here: https://github.com/getsentry/sentry-docs/blob/master/src/gatsby/onCreateWebpackConfig.ts#L15
So this should still work the same, actually.
I removed the manually set
release, letting this being set by the webpack plugin (hopefully). The DSN is still read fromGATSBY_SENTRY_DSN, the same as before.We still depend on
@sentry/browser, but only for the types.Closes #6884