chore(react): cleanup outstanding TODOs on @sentry/react (#2661)

* feat(react): Add Event Processor for React
* chore(react): Update comments and test for Profiler
* fix(react): Correctly give eventID to dialog
* fix(react): Prevent state updates from creating activities

Author: AbhiPrasad

packages/react/package.json

@@ -32,6 +32,7 @@
     "@types/hoist-non-react-statics": "^3.3.1",
     "@types/react": "^16.9.35",
     "jest": "^24.7.1",
+    "jsdom": "^16.2.2",
     "npm-run-all": "^4.1.2",
     "prettier": "^1.17.0",
     "prettier-check": "^2.0.0",

packages/react/src/errorboundary.tsx

@@ -11,13 +11,31 @@ export type FallbackRender = (fallback: {
 }) => React.ReactNode;
 
 export type ErrorBoundaryProps = {
+  /** If a Sentry report dialog should be rendered on error */
   showDialog?: boolean;
+  /**
+   * Options to be passed into the Sentry report dialog.
+   * No-op if {@link showDialog} is false.
+   */
   dialogOptions?: Sentry.ReportDialogOptions;
-  // tslint:disable-next-line: no-null-undefined-union
+  // tslint:disable no-null-undefined-union
+  /**
+   * A fallback component that gets rendered when the error boundary encounters an error.
+   *
+   * Can either provide a React Component, or a function that returns React Component as
+   * a valid fallback prop. If a function is provided, the function will be called with
+   * the error, the component stack, and an function that resets the error boundary on error.
+   *
+   */
   fallback?: React.ReactNode | FallbackRender;
+  // tslint:enable no-null-undefined-union
+  /** Called with the error boundary encounters an error */
   onError?(error: Error, componentStack: string): void;
+  /** Called on componentDidMount() */
   onMount?(): void;
+  /** Called if resetError() is called from the fallback render props function  */
   onReset?(error: Error | null, componentStack: string | null): void;
+  /** Called on componentWillUnmount() */
   onUnmount?(error: Error | null, componentStack: string | null): void;
 };
 
@@ -31,17 +49,21 @@ const INITIAL_STATE = {
   error: null,
 };
 
+/**
+ * A ErrorBoundary component that logs errors to Sentry.
+ * Requires React >= 16
+ */
 class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundaryState> {
   public state: ErrorBoundaryState = INITIAL_STATE;
 
   public componentDidCatch(error: Error, { componentStack }: React.ErrorInfo): void {
-    Sentry.captureException(error, { contexts: { react: { componentStack } } });
+    const eventId = Sentry.captureException(error, { contexts: { react: { componentStack } } });
     const { onError, showDialog, dialogOptions } = this.props;
     if (onError) {
       onError(error, componentStack);
     }
     if (showDialog) {
-      Sentry.showReportDialog(dialogOptions);
+      Sentry.showReportDialog({ ...dialogOptions, eventId });
     }
 
     // componentDidCatch is used over getDerivedStateFromError

packages/react/src/index.ts

@@ -1,4 +1,27 @@
+import { addGlobalEventProcessor, SDK_VERSION } from '@sentry/browser';
+
+function createReactEventProcessor(): void {
+  addGlobalEventProcessor(event => {
+    event.sdk = {
+      ...event.sdk,
+      name: 'sentry.javascript.react',
+      packages: [
+        ...((event.sdk && event.sdk.packages) || []),
+        {
+          name: 'npm:@sentry/react',
+          version: SDK_VERSION,
+        },
+      ],
+      version: SDK_VERSION,
+    };
+
+    return event;
+  });
+}
+
 export * from '@sentry/browser';
 
 export { Profiler, withProfiler, useProfiler } from './profiler';
 export { ErrorBoundary, withErrorBoundary } from './errorboundary';
+
+createReactEventProcessor();

packages/react/src/profiler.tsx

@@ -39,6 +39,10 @@ function afterNextFrame(callback: Function): void {
   timeout = window.setTimeout(done, 100);
 }
 
+/**
+ * getInitActivity pushes activity based on React component mount
+ * @param name displayName of component that started activity
+ */
 const getInitActivity = (name: string): number | null => {
   const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
 
@@ -68,10 +72,13 @@ class Profiler extends React.Component<ProfilerProps> {
     this.activity = getInitActivity(this.props.name);
   }
 
+  // If a component mounted, we can finish the mount activity.
   public componentDidMount(): void {
     afterNextFrame(this.finishProfile);
   }
 
+  // Sometimes a component will unmount first, so we make
+  // sure to also finish the mount activity here.
   public componentWillUnmount(): void {
     afterNextFrame(this.finishProfile);
   }
@@ -94,8 +101,15 @@ class Profiler extends React.Component<ProfilerProps> {
   }
 }
 
-function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>): React.FC<P> {
-  const componentDisplayName = WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
+/**
+ * withProfiler is a higher order component that wraps a
+ * component in a {@link Profiler} component.
+ *
+ * @param WrappedComponent component that is wrapped by Profiler
+ * @param name displayName of component being profiled
+ */
+function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>, name?: string): React.FC<P> {
+  const componentDisplayName = name || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
 
   const Wrapped: React.FC<P> = (props: P) => (
     <Profiler name={componentDisplayName}>
@@ -119,7 +133,7 @@ function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>
  * @param name displayName of component being profiled
  */
 function useProfiler(name: string): void {
-  const activity = getInitActivity(name);
+  const [activity] = React.useState(() => getInitActivity(name));
 
   React.useEffect(() => {
     afterNextFrame(() => {

packages/react/test/errorboundary.test.tsx

@@ -1,14 +1,16 @@
 import { fireEvent, render, screen } from '@testing-library/react';
 import * as React from 'react';
 
-import { ErrorBoundary, ErrorBoundaryProps } from '../src/errorboundary';
+import { ErrorBoundary, ErrorBoundaryProps, UNKNOWN_COMPONENT, withErrorBoundary } from '../src/errorboundary';
 
 const mockCaptureException = jest.fn();
 const mockShowReportDialog = jest.fn();
+const EVENT_ID = 'test-id-123';
 
 jest.mock('@sentry/browser', () => ({
   captureException: (err: any, ctx: any) => {
     mockCaptureException(err, ctx);
+    return EVENT_ID;
   },
   showReportDialog: (options: any) => {
     mockShowReportDialog(options);
@@ -18,7 +20,15 @@ jest.mock('@sentry/browser', () => ({
 const TestApp: React.FC<ErrorBoundaryProps> = ({ children, ...props }) => {
   const [isError, setError] = React.useState(false);
   return (
-    <ErrorBoundary {...props}>
+    <ErrorBoundary
+      {...props}
+      onReset={(err: Error, stack: string) => {
+        setError(false);
+        if (props.onReset) {
+          props.onReset(err, stack);
+        }
+      }}
+    >
       {isError ? <Bam /> : children}
       <button
         data-testid="errorBtn"
@@ -34,6 +44,20 @@ function Bam(): JSX.Element {
   throw new Error('boom');
 }
 
+describe('withErrorBoundary', () => {
+  it('sets displayName properly', () => {
+    const TestComponent = () => <h1>Hello World</h1>;
+
+    const Component = withErrorBoundary(TestComponent, { fallback: <h1>fallback</h1> });
+    expect(Component.displayName).toBe('errorBoundary(TestComponent)');
+  });
+
+  it('defaults to an unknown displayName', () => {
+    const Component = withErrorBoundary(() => <h1>Hello World</h1>, { fallback: <h1>fallback</h1> });
+    expect(Component.displayName).toBe(`errorBoundary(${UNKNOWN_COMPONENT})`);
+  });
+});
+
 describe('ErrorBoundary', () => {
   jest.spyOn(console, 'error').mockImplementation();
 
@@ -44,7 +68,6 @@ describe('ErrorBoundary', () => {
 
   it('renders null if not given a valid `fallback` prop', () => {
     const { container } = render(
-      // @ts-ignore
       <ErrorBoundary fallback={new Error('true')}>
         <Bam />
       </ErrorBoundary>,
@@ -55,7 +78,6 @@ describe('ErrorBoundary', () => {
 
   it('renders a fallback on error', () => {
     const { container } = render(
-      // @ts-ignore
       <ErrorBoundary fallback={<h1>Error Component</h1>}>
         <Bam />
       </ErrorBoundary>,
@@ -186,12 +208,30 @@ describe('ErrorBoundary', () => {
       fireEvent.click(btn);
 
       expect(mockShowReportDialog).toHaveBeenCalledTimes(1);
-      expect(mockShowReportDialog).toHaveBeenCalledWith(options);
+      expect(mockShowReportDialog).toHaveBeenCalledWith({ ...options, eventId: EVENT_ID });
     });
 
-    it('resets to initial state when reset', () => {
-      const mockOnReset = jest.fn();
+    it('resets to initial state when reset', async () => {
       const { container } = render(
+        <TestApp fallback={({ resetError }) => <button data-testid="reset" onClick={resetError} />}>
+          <h1>children</h1>
+        </TestApp>,
+      );
+
+      expect(container.innerHTML).toContain('<h1>children</h1>');
+      const btn = screen.getByTestId('errorBtn');
+      fireEvent.click(btn);
+      expect(container.innerHTML).toContain('<button data-testid="reset">');
+
+      const reset = screen.getByTestId('reset');
+      fireEvent.click(reset);
+
+      expect(container.innerHTML).toContain('<h1>children</h1>');
+    });
+
+    it('calls `onReset()` when reset', () => {
+      const mockOnReset = jest.fn();
+      render(
         <TestApp
           onReset={mockOnReset}
           fallback={({ resetError }) => <button data-testid="reset" onClick={resetError} />}
@@ -200,17 +240,14 @@ describe('ErrorBoundary', () => {
         </TestApp>,
       );
 
-      expect(container.innerHTML).toContain('<h1>children</h1>');
       expect(mockOnReset).toHaveBeenCalledTimes(0);
-
       const btn = screen.getByTestId('errorBtn');
       fireEvent.click(btn);
-
-      expect(container.innerHTML).toContain('<button data-testid="reset">');
       expect(mockOnReset).toHaveBeenCalledTimes(0);
 
       const reset = screen.getByTestId('reset');
       fireEvent.click(reset);
+
       expect(mockOnReset).toHaveBeenCalledTimes(1);
       expect(mockOnReset).toHaveBeenCalledWith(expect.any(Error), expect.any(String));
     });

packages/react/test/profiler.test.tsx

@@ -6,6 +6,17 @@ import { UNKNOWN_COMPONENT, useProfiler, withProfiler } from '../src/profiler';
 
 const mockPushActivity = jest.fn().mockReturnValue(1);
 const mockPopActivity = jest.fn();
+const mockLoggerWarn = jest.fn();
+
+let integrationIsNull = false;
+
+jest.mock('@sentry/utils', () => ({
+  logger: {
+    warn: (message: string) => {
+      mockLoggerWarn(message);
+    },
+  },
+}));
 
 jest.mock('@sentry/browser', () => ({
   getCurrentHub: () => ({
@@ -20,7 +31,11 @@ jest.mock('@sentry/browser', () => ({
         public static popActivity: () => void = mockPopActivity;
       }
 
-      return new MockIntegration('test');
+      if (!integrationIsNull) {
+        return new MockIntegration('test');
+      }
+
+      return null;
     },
   }),
 }));
@@ -30,6 +45,8 @@ describe('withProfiler', () => {
     jest.useFakeTimers();
     mockPushActivity.mockClear();
     mockPopActivity.mockClear();
+    mockLoggerWarn.mockClear();
+    integrationIsNull = false;
   });
 
   it('sets displayName properly', () => {
@@ -39,6 +56,18 @@ describe('withProfiler', () => {
     expect(ProfiledComponent.displayName).toBe('profiler(TestComponent)');
   });
 
+  it('sets a custom displayName', () => {
+    const TestComponent = () => <h1>Hello World</h1>;
+
+    const ProfiledComponent = withProfiler(TestComponent, 'BestComponent');
+    expect(ProfiledComponent.displayName).toBe('profiler(BestComponent)');
+  });
+
+  it('defaults to an unknown displayName', () => {
+    const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>);
+    expect(ProfiledComponent.displayName).toBe(`profiler(${UNKNOWN_COMPONENT})`);
+  });
+
   it('popActivity() is called when unmounted', () => {
     const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>);
 
@@ -63,13 +92,32 @@ describe('withProfiler', () => {
       op: 'react',
     });
   });
+
+  it('does not start an activity when integration is disabled', () => {
+    integrationIsNull = true;
+    const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>);
+
+    expect(mockPushActivity).toHaveBeenCalledTimes(0);
+    expect(mockLoggerWarn).toHaveBeenCalledTimes(0);
+
+    const profiler = render(<ProfiledComponent />);
+    expect(mockPopActivity).toHaveBeenCalledTimes(0);
+    expect(mockPushActivity).toHaveBeenCalledTimes(0);
+
+    expect(mockLoggerWarn).toHaveBeenCalledTimes(1);
+
+    profiler.unmount();
+    expect(mockPopActivity).toHaveBeenCalledTimes(0);
+  });
 });
 
 describe('useProfiler()', () => {
   beforeEach(() => {
     jest.useFakeTimers();
     mockPushActivity.mockClear();
     mockPopActivity.mockClear();
+    mockLoggerWarn.mockClear();
+    integrationIsNull = false;
   });
 
   it('popActivity() is called when unmounted', () => {
@@ -95,4 +143,20 @@ describe('useProfiler()', () => {
       op: 'react',
     });
   });
+
+  it('does not start an activity when integration is disabled', () => {
+    integrationIsNull = true;
+    expect(mockPushActivity).toHaveBeenCalledTimes(0);
+    expect(mockLoggerWarn).toHaveBeenCalledTimes(0);
+
+    // tslint:disable-next-line: no-void-expression
+    const profiler = renderHook(() => useProfiler('Example'));
+    expect(mockPopActivity).toHaveBeenCalledTimes(0);
+    expect(mockPushActivity).toHaveBeenCalledTimes(0);
+
+    expect(mockLoggerWarn).toHaveBeenCalledTimes(1);
+
+    profiler.unmount();
+    expect(mockPopActivity).toHaveBeenCalledTimes(0);
+  });
 });