ref(react): Check if React span is finished

Author: AbhiPrasad

packages/react/package.json

@@ -19,14 +19,15 @@
     "@sentry/browser": "5.16.0-beta.5",
     "@sentry/types": "5.16.0-beta.5",
     "@sentry/utils": "^5.15.5",
+    "hoist-non-react-statics": "^3.3.2",
     "tslib": "^1.9.3"
   },
   "peerDependencies": {
     "react": "^16.0.0",
     "react-dom": "^16.0.0"
   },
   "devDependencies": {
-    "@testing-library/react": "^10.0.4",
+    "@types/hoist-non-react-statics": "^3.3.1",
     "@types/react": "^16.9.35",
     "@types/react-test-renderer": "^16.9.2",
     "jest": "^24.7.1",

packages/react/src/profiler.tsx

@@ -1,31 +1,57 @@
 import { getCurrentHub } from '@sentry/browser';
 import { Integration, IntegrationClass } from '@sentry/types';
 import { logger } from '@sentry/utils';
+import * as hoistNonReactStatic from 'hoist-non-react-statics';
 import * as React from 'react';
 
-export const DEFAULT_DURATION = 30000;
 export const UNKNOWN_COMPONENT = 'unknown';
 
 const TRACING_GETTER = ({
   id: 'Tracing',
 } as any) as IntegrationClass<Integration>;
 
-const getInitActivity = (componentDisplayName: string, timeout: number): number | null => {
+/**
+ *
+ * Based on implementation from Preact:
+ * https:github.com/preactjs/preact/blob/9a422017fec6dab287c77c3aef63c7b2fef0c7e1/hooks/src/index.js#L301-L313
+ *
+ * Schedule a callback to be invoked after the browser has a chance to paint a new frame.
+ * Do this by combining requestAnimationFrame (rAF) + setTimeout to invoke a callback after
+ * the next browser frame.
+ *
+ * Also, schedule a timeout in parallel to the the rAF to ensure the callback is invoked
+ * even if RAF doesn't fire (for example if the browser tab is not visible)
+ *
+ * This is what we use to tell if a component activity has finished
+ *
+ */
+function afterNextFrame(callback: Function): void {
+  let timeout: number | undefined;
+  let raf: number;
+
+  const done = () => {
+    window.clearTimeout(timeout);
+    window.cancelAnimationFrame(raf);
+    window.setTimeout(callback);
+  };
+
+  raf = window.requestAnimationFrame(done);
+  timeout = window.setTimeout(done, 100);
+}
+
+const getInitActivity = (componentDisplayName: string): number | null => {
   const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
 
   if (tracingIntegration !== null) {
     // tslint:disable-next-line:no-unsafe-any
-    return (tracingIntegration as any).constructor.pushActivity(
-      componentDisplayName,
-      {
-        data: {},
-        description: `<${componentDisplayName}>`,
-        op: 'react',
-      },
-      {
-        autoPopAfter: timeout,
-      },
-    );
+    const activity = (tracingIntegration as any).constructor.pushActivity(componentDisplayName, {
+      data: {},
+      description: `<${componentDisplayName}>`,
+      op: 'react',
+    });
+
+    // tslint:disable-next-line: no-unsafe-any
+    return activity;
   }
 
   logger.warn(`Unable to profile component ${componentDisplayName} due to invalid Tracing Integration`);
@@ -34,7 +60,6 @@ const getInitActivity = (componentDisplayName: string, timeout: number): number
 
 interface ProfilerProps {
   componentDisplayName?: string;
-  timeout?: number;
 }
 
 interface ProfilerState {
@@ -45,15 +70,23 @@ class Profiler extends React.Component<ProfilerProps, ProfilerState> {
   public constructor(props: ProfilerProps) {
     super(props);
 
-    const { componentDisplayName = UNKNOWN_COMPONENT, timeout = DEFAULT_DURATION } = this.props;
+    const { componentDisplayName = UNKNOWN_COMPONENT } = this.props;
 
     this.state = {
-      activity: getInitActivity(componentDisplayName, timeout),
+      activity: getInitActivity(componentDisplayName),
     };
   }
 
+  public componentDidMount(): void {
+    if (this.state.activity) {
+      afterNextFrame(this.finishProfile);
+    }
+  }
+
   public componentWillUnmount(): void {
-    this.finishProfile();
+    if (this.state.activity) {
+      afterNextFrame(this.finishProfile);
+    }
   }
 
   public finishProfile = () => {
@@ -88,6 +121,9 @@ function withProfiler<P extends object>(
 
   Wrapped.displayName = `profiler(${componentDisplayName})`;
 
+  // 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);
   return Wrapped;
 }
 

packages/react/test/profiler.test.tsx

@@ -1,7 +1,7 @@
 import * as React from 'react';
 import { create } from 'react-test-renderer';
 
-import { DEFAULT_DURATION, UNKNOWN_COMPONENT, withProfiler } from '../src/profiler';
+import { UNKNOWN_COMPONENT, withProfiler } from '../src/profiler';
 
 const mockPushActivity = jest.fn().mockReturnValue(1);
 const mockPopActivity = jest.fn();
@@ -57,35 +57,18 @@ describe('withProfiler', () => {
         expect(mockPushActivity).toHaveBeenCalledTimes(0);
         create(<ProfiledComponent />);
         expect(mockPushActivity).toHaveBeenCalledTimes(1);
-        expect(mockPushActivity).toHaveBeenLastCalledWith(
-          UNKNOWN_COMPONENT,
-          {
-            data: {},
-            description: `<${UNKNOWN_COMPONENT}>`,
-            op: 'react',
-          },
-          { autoPopAfter: DEFAULT_DURATION },
-        );
-      });
-
-      it('is called with a custom timeout', () => {
-        const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>, { timeout: 32 });
-
-        create(<ProfiledComponent />);
-        expect(mockPushActivity).toHaveBeenLastCalledWith(expect.any(String), expect.any(Object), {
-          autoPopAfter: 32,
+        expect(mockPushActivity).toHaveBeenLastCalledWith(UNKNOWN_COMPONENT, {
+          data: {},
+          description: `<${UNKNOWN_COMPONENT}>`,
+          op: 'react',
         });
       });
 
       it('is called with a custom displayName', () => {
         const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>, { componentDisplayName: 'Test' });
 
         create(<ProfiledComponent />);
-        expect(mockPushActivity).toHaveBeenLastCalledWith(
-          'Test',
-          expect.objectContaining({ description: '<Test>' }),
-          expect.any(Object),
-        );
+        expect(mockPushActivity).toHaveBeenLastCalledWith('Test', expect.objectContaining({ description: '<Test>' }));
       });
     });
   });