-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Add @sentry/react #2631
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/react #2631
Conversation
8765508 to
bee7c05
Compare
HazAT
left a comment
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.
Did the first pass, really like it :)
Just a few general things to remember
- Update PR description with Motivation + Description
- Add Changelog Entry
05e59b4 to
9acad74
Compare
|
|
||
| // Copy over static methods from Wrapped component to Profiler HOC | ||
| // See: https://reactjs.org/docs/higher-order-components.html#static-methods-must-be-copied-over | ||
| hoistNonReactStatic(Wrapped, WrappedComponent); |
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've decided to keep this library for now instead of vendoring it in. This is as it leverages functionality from react-is, https://github.com/facebook/react/tree/master/packages/react-is, which is hard to keep up to date properly (lot's of manual copy-paste, hard to know when different react types will change).
Many popular libraries use hoist-non-react-statics. See:
react-intl: https://github.com/formatjs/formatjs/blob/master/packages/react-intl/package.json#L143
react-redux: https://github.com/reduxjs/react-redux/blob/master/package.json#L52
react-apollo: https://github.com/apollographql/react-apollo/blob/master/packages/hoc/package.json#L45
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 it's worth the bundle size change here for a much smoother experience, it adds 1.2kb gzipped + minified - https://bundlephobia.com/[email protected]
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.
OK, let's keep it for now.
HazAT
left a comment
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.
👍
A few nits, otherwise LGTM
|
|
||
| // Copy over static methods from Wrapped component to Profiler HOC | ||
| // See: https://reactjs.org/docs/higher-order-components.html#static-methods-must-be-copied-over | ||
| hoistNonReactStatic(Wrapped, WrappedComponent); |
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.
OK, let's keep it for now.
Co-authored-by: Daniel Griesser <[email protected]>
0df029f to
73e8dce
Compare
73e8dce to
cf8d8ca
Compare
Motivation
This PR adds the
@sentry/reactpackage. This provides our users with an easy way to use@sentry/browsercombined with some custom React components.The plan is to have a
Profilercomponent (and according HOC) that works with@sentry/apmto provide per component tracing, and a custom<Sentry.ErrorBoundary />component that leverages React 16's error boundary functionality. The Profiler will both have a hooks based, and class based implementation.Implementation
A new lerna package
@sentry/reactwas created. For the purpose of this first PR, a class basedProfilercomponent was created. This component can be used as such:The Profiler has one prop,
componentDisplayName, which allows the user to customize what the display name of the profiler.Testing
Tests were written leveraging the
react-test-rendererlibrary. I did this because I needed to test specific lifecycle functionality, and didn't want the added weight of usingenzymeorreact-testing-library.In addition, this library was tested on
getsentry/sentryand various demo repo's. I can provide links to example traces upon request, just ping me.This class based Profiler should work with React >= 15
Future
After this PR is merged, we will look toward creating the
ErrorBoundarycomponent.