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/react #2631

Merged
merged 11 commits into from Jun 3, 2020
Merged

feat: Add @sentry/react #2631

merged 11 commits into from Jun 3, 2020

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Jun 1, 2020

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 based Profiler component was created. This component can be used as such:

import * as Sentry from '@sentry/react';
...
// Using Profiler component
render() {
  <Profiler>
    <Component />
  </Profiler>
}

...
// Using HOC
function App: React.FC<>

return withProfiler(App)

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 using enzyme or react-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.

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.

Did the first pass, really like it :)

Just a few general things to remember

  • Update PR description with Motivation + Description
  • Add Changelog Entry

packages/react/src/index.ts Outdated Show resolved Hide resolved
packages/react/rollup.config.js Outdated Show resolved Hide resolved
packages/react/src/profiler.tsx Outdated Show resolved Hide resolved
packages/react/package.json Outdated Show resolved Hide resolved
packages/react/package.json Outdated Show resolved Hide resolved
packages/react/package.json Outdated Show resolved Hide resolved
@AbhiPrasad AbhiPrasad force-pushed the abhi/feat/react branch 2 times, most recently from 05e59b4 to 9acad74 Compare June 2, 2020 14:32
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jun 2, 2020

Messages
📖

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

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against cf8d8ca


// 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);
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'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

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

Copy link
Member

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.

@AbhiPrasad AbhiPrasad changed the title [WIP] feat: Add @sentry/react feat: Add @sentry/react Jun 2, 2020
@AbhiPrasad AbhiPrasad marked this pull request as ready for review June 2, 2020 16:34
@AbhiPrasad AbhiPrasad requested a review from HazAT June 2, 2020 16:34
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.

👍
A few nits, otherwise LGTM

packages/react/package.json Outdated Show resolved Hide resolved

// 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);
Copy link
Member

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.

packages/react/src/profiler.tsx Outdated Show resolved Hide resolved
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

3 participants