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: Add @sentry/gatsby #2652

Merged
merged 2 commits into from Jun 10, 2020
Merged

feat: Add @sentry/gatsby #2652

merged 2 commits into from Jun 10, 2020

Conversation

dcramer
Copy link
Member

@dcramer dcramer commented Jun 5, 2020

No description provided.

@dcramer
Copy link
Member Author

dcramer commented Jun 5, 2020

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.

@dcramer
Copy link
Member Author

dcramer commented Jun 5, 2020

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.

@dcramer dcramer force-pushed the feat/gatsby branch 2 times, most recently from 5b133db to 756812d Compare June 5, 2020 20:58
@dcramer
Copy link
Member Author

dcramer commented Jun 5, 2020

Cool looks like this pattern works

@dcramer dcramer marked this pull request as ready for review June 5, 2020 21:04
@dcramer dcramer requested a review from kamilogorek as a code owner June 5, 2020 21:04
process.env.ZEIT_BITBUCKET_COMMIT_SHA ||
'',
),
__SENTRY_DSN__: JSON.stringify(process.env.SENTRY_DSN || ''),
Copy link
Member

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

Copy link
Member

@HazAT HazAT left a 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 👍

Comment on lines 19 to 21
"@sentry/browser": "5.16.1",
"@sentry/types": "5.16.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"@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)?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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": "*"
Copy link
Member

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?

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 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.

packages/gatsby/package.json Show resolved Hide resolved
process.env.ZEIT_GITHUB_COMMIT_SHA ||
process.env.ZEIT_GITLAB_COMMIT_SHA ||
process.env.ZEIT_BITBUCKET_COMMIT_SHA ||
'',
Copy link
Member

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?

Copy link
Member Author

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.

packages/gatsby/package.json Show resolved Hide resolved
@dcramer dcramer force-pushed the feat/gatsby branch 2 times, most recently from e4141ae to 1ae8b14 Compare June 8, 2020 15:38
@dcramer
Copy link
Member Author

dcramer commented Jun 9, 2020

what else do we need here to get this merged?

Copy link
Member

@AbhiPrasad AbhiPrasad left a 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.

"access": "public"
},
"dependencies": {
"@sentry/browser": "5.16.1",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"@sentry/browser": "5.16.1",
"@sentry/react": "5.17.0",

@@ -0,0 +1,78 @@
{
"name": "@sentry/gatsby",
"version": "5.16.1",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"version": "5.16.1",
"version": "5.17.0",

Copy link
Member

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

@dcramer
Copy link
Member Author

dcramer commented Jun 9, 2020

Any reason we dont just use 0.0.0 in package.json and automatically change them in releases?

Copy link
Member

@HazAT HazAT left a 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.

packages/gatsby/package.json Outdated Show resolved Hide resolved
packages/gatsby/src/gatsby-browser.js Outdated Show resolved Hide resolved
@dcramer
Copy link
Member Author

dcramer commented Jun 10, 2020

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

@dcramer dcramer force-pushed the feat/gatsby branch 2 times, most recently from 64155cc to c751c9c Compare June 10, 2020 17:27
@dcramer
Copy link
Member Author

dcramer commented Jun 10, 2020

I couldn't get tests passing locally.. @sentry/react (based on this import) wasn't exporting some of the things that it clearly exports in its index.ts file. That means its either broken (I doubt that), or I have no idea what I'm doing :)

Hopefully passes in CI!

@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jun 10, 2020

Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 17.083 kB) (ES6: 16.1533 kB)

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against a146fdc

@dcramer dcramer merged commit e8093c2 into master Jun 10, 2020
@dcramer dcramer deleted the feat/gatsby branch June 10, 2020 20:56
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

5 participants