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

ref: Use @sentry/react #19366

Merged
merged 8 commits into from Jun 25, 2020
Merged

ref: Use @sentry/react #19366

merged 8 commits into from Jun 25, 2020

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Jun 15, 2020

Motivation

As we are preparing to add public docs for https://github.com/getsentry/sentry-javascript/tree/master/packages/react, we figured it would be best to dogfood the react package first with sentry.

@sentry/react is a superset of the @sentry/browser package, with a custom Profiler component added, as well as an ErrorBoundary component out of the box.

Implementation

List of changes:

  1. Replace all instances of @sentry/browser with @sentry/react
  2. Delete built in profiler, use withProfiler from @sentry/react

Future

Another PR to add the @sentry/react ErrorBoundary component.

@vercel vercel bot temporarily deployed to Preview June 16, 2020 20:50 Inactive
@vercel vercel bot temporarily deployed to Preview June 17, 2020 11:49 Inactive
@AbhiPrasad AbhiPrasad marked this pull request as ready for review June 25, 2020 15:16
@AbhiPrasad AbhiPrasad requested a review from a team as a code owner June 25, 2020 15:16
@AbhiPrasad AbhiPrasad requested review from a team and HazAT June 25, 2020 15:16
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

🚀

Left a comment re: sdk API, but otherwise looks good

@@ -82,4 +80,6 @@ LoadingIndicator.propTypes = {
hideSpinner: PropTypes.bool,
};

export default withProfiler(LoadingIndicator);
export default withProfiler(LoadingIndicator, {
hasUpdateSpan: false,
Copy link
Member

Choose a reason for hiding this comment

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

mmm I can't form words atm, but has doesn't read to me that this is an option for withProfiler... maybe something like includeUpdateSpan?

Copy link
Member

Choose a reason for hiding this comment

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

Or just updateSpan

Copy link
Member

Choose a reason for hiding this comment

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

updateSpan makes it sound like the profiler will perform an update of a span -- maybe includeUpdates?

Copy link
Member Author

Choose a reason for hiding this comment

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

So somethin alongs the lines of:

// If time component is on page should be displayed as a span. True by default.
includeRender?: boolean;
// If component updates should be displayed as spans. True by default.
includeUpdates?: boolean;

I like this naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I am going to merge this in, but I will make the API changes and come back and update.

@AbhiPrasad AbhiPrasad merged commit e540f7c into master Jun 25, 2020
@AbhiPrasad AbhiPrasad deleted the abhi/feat/use-sentry-react branch June 25, 2020 20:17
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants