-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Add @sentry/gatsby #2652
feat: Add @sentry/gatsby #2652
Conversation
Still confirming this all works as expected via some other projects. I'd love to get automated tests in here, but I'm not entirely sure how without a huge boilerplate project (which may not be what we want). I also didn't test Netlify, and just based it on their docs. |
If ya'll have any initial feedback on project layout/core stuff let me know. I based it on apm/react, and I'm not sure if I can remove any of the stuff from package.json. |
5b133db
to
756812d
Compare
Cool looks like this pattern works |
process.env.ZEIT_BITBUCKET_COMMIT_SHA || | ||
'', | ||
), | ||
__SENTRY_DSN__: JSON.stringify(process.env.SENTRY_DSN || ''), |
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.
@dcramer I hope this doesn't pick the wrong DSN with a Vercel project. They have some hack in the next JS Sentry example because I presume they have an internal DSN for Sentry that conflicts: https://github.com/vercel/next.js/blob/canary/examples/with-sentry-simple/next.config.js#L8
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.
This should go in the index.ts
file
export * from '@sentry/react';
I am not sure how Gatsby works but in theory, users would be able to use the profiler component + ErrorBoundary of our react package.
cc @AbhiPrasad
Otherwise, nice 👍
packages/gatsby/package.json
Outdated
"@sentry/browser": "5.16.1", | ||
"@sentry/types": "5.16.1" |
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.
"@sentry/browser": "5.16.1", | |
"@sentry/types": "5.16.1" | |
"@sentry/apm": "5.16.1", | |
"@sentry/react": "5.16.1", | |
"@sentry/types": "5.16.1" |
Missing reference to @sentry/apm
Also, shouldn't we be able to have @sentry/react
as a base (vs. @sentry/browser
)?
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.
Yes @sentry/react
can be used wherever @sentry/browser
is used, the react package is a superset of the browser package (https://github.com/getsentry/sentry-javascript/blob/master/packages/react/src/index.ts#L1).
Also, we shouldn't need @sentry/types
right? Right now the gatsby exported files don't use TS at all, so a user can just install @sentry/types
manually if they want types.
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.
It probably also makes sense then to makes docs changes + announcement of @sentry/react
and @sentry/gatsby
at the same time.
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.
Yep, we can officially publish them at the same time.
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.
sentry/react makes sense - though theres a possibility in the future it could make sense to inject sentry during compilation which is node.js. ks there actually anything I need to change in that case?
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.
We can just require("@sentry/node")
in gatsby-node.js
right? Don't think it should change what we have right now.
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.
👍 yeah wasn't sure
"@sentry/types": "5.16.1" | ||
}, | ||
"peerDependencies": { | ||
"gatsby": "*" |
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.
This config will work for all gatsby versions?
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 dont know what versions itll work for, and I don't really want to dig through Gatsby history to figure out when specific APIs were created/etc.
process.env.ZEIT_GITHUB_COMMIT_SHA || | ||
process.env.ZEIT_GITLAB_COMMIT_SHA || | ||
process.env.ZEIT_BITBUCKET_COMMIT_SHA || | ||
'', |
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.
Is there no way for us to make this a user set option without relying on environmental variables? Can we provide some kind of pluginOptions
like they can with onClientEntry
and pluginParams
?
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.
this is still able to be overwritten from pluginOptions, but we should not make a user set anything unless they absolutely have to. frictionless has always been the goal.
e4141ae
to
1ae8b14
Compare
what else do we need here to get this merged? |
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.
We bumped to 5.17.0
so we will need to use that.
packages/gatsby/package.json
Outdated
"access": "public" | ||
}, | ||
"dependencies": { | ||
"@sentry/browser": "5.16.1", |
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.
"@sentry/browser": "5.16.1", | |
"@sentry/react": "5.17.0", |
packages/gatsby/package.json
Outdated
@@ -0,0 +1,78 @@ | |||
{ | |||
"name": "@sentry/gatsby", | |||
"version": "5.16.1", |
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.
"version": "5.16.1", | |
"version": "5.17.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.
ditto wherever 5.16.1
is used
Any reason we dont just use 0.0.0 in package.json and automatically change them in releases? |
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.
Forgot this, we need to extend from the react package then we are golden.
I'm going to add a baseline of module level export tests ("does it export react helpers"), if nothing else so we have a functional test suite |
64155cc
to
c751c9c
Compare
I couldn't get tests passing locally.. Hopefully passes in CI! |
No description provided.