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
feat: Add @sentry/react #2631
Conversation
8765508
to
bee7c05
Compare
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/result?p=hoist-non-react-statics@3.3.2
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.
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 <daniel.griesser.86@gmail.com>
0df029f
to
73e8dce
Compare
73e8dce
to
cf8d8ca
Compare
Motivation
This PR adds the
@sentry/react
package. This provides our users with an easy way to use@sentry/browser
combined with some custom React components.The plan is to have a
Profiler
component (and according HOC) that works with@sentry/apm
to 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/react
was created. For the purpose of this first PR, a class basedProfiler
component 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-renderer
library. I did this because I needed to test specific lifecycle functionality, and didn't want the added weight of usingenzyme
orreact-testing-library
.In addition, this library was tested on
getsentry/sentry
and 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
ErrorBoundary
component.