From d926d7fb911ca9fc56bfea4ff14948bbae2ff51c Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Fri, 5 Aug 2022 15:11:28 -0400 Subject: [PATCH 1/7] Track duration of React component updates --- packages/react/src/profiler.tsx | 59 ++++++++++++++++++--------- packages/react/test/profiler.test.tsx | 55 ++++++++++++++----------- 2 files changed, 70 insertions(+), 44 deletions(-) diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx index c52d503eb92b..2a8b1bdc0b83 100644 --- a/packages/react/src/profiler.tsx +++ b/packages/react/src/profiler.tsx @@ -1,12 +1,12 @@ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { getCurrentHub, Hub } from '@sentry/browser'; -import { Span, Transaction } from '@sentry/types'; -import { timestampWithMs } from '@sentry/utils'; +import {getCurrentHub, Hub} from '@sentry/browser'; +import {Span, Transaction} from '@sentry/types'; +import {timestampWithMs} from '@sentry/utils'; import hoistNonReactStatics from 'hoist-non-react-statics'; import * as React from 'react'; -import { REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP } from './constants'; +import {REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP} from './constants'; export const UNKNOWN_COMPONENT = 'unknown'; @@ -21,7 +21,7 @@ export type ProfilerProps = { // If component updates should be displayed as spans. True by default. includeUpdates?: boolean; // props given to component being profiled. - updateProps: { [key: string]: unknown }; + updateProps: {[key: string]: unknown}; }; /** @@ -34,6 +34,7 @@ class Profiler extends React.Component { * Made protected for the React Native SDK to access */ protected _mountSpan: Span | undefined = undefined; + protected _updateSpan: Span | undefined = undefined; // eslint-disable-next-line @typescript-eslint/member-ordering public static defaultProps: Partial = { @@ -44,7 +45,7 @@ class Profiler extends React.Component { public constructor(props: ProfilerProps) { super(props); - const { name, disabled = false } = this.props; + const {name, disabled = false} = this.props; if (disabled) { return; @@ -66,35 +67,48 @@ class Profiler extends React.Component { } } - public componentDidUpdate({ updateProps, includeUpdates = true }: ProfilerProps): void { - // Only generate an update span if hasUpdateSpan is true, if there is a valid mountSpan, - // and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive. - // We are just trying to give baseline clues for further investigation. + public shouldComponentUpdate({ + updateProps, + includeUpdates = true, + }: ProfilerProps): boolean { if (includeUpdates && this._mountSpan && updateProps !== this.props.updateProps) { // See what props haved changed between the previous props, and the current props. This is // set as data on the span. We just store the prop keys as the values could be potenially very large. - const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]); + const changedProps = Object.keys(updateProps).filter( + k => updateProps[k] !== this.props.updateProps[k] + ); if (changedProps.length > 0) { // The update span is a point in time span with 0 duration, just signifying that the component // has been updated. const now = timestampWithMs(); - this._mountSpan.startChild({ + this._updateSpan = this._mountSpan.startChild({ data: { changedProps, }, description: `<${this.props.name}>`, - endTimestamp: now, op: REACT_UPDATE_OP, startTimestamp: now, }); } } + + return true; + } + + public componentDidUpdate(): void { + // Only generate an update span if hasUpdateSpan is true, if there is a valid mountSpan, + // and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive. + // We are just trying to give baseline clues for further investigation. + if (this._updateSpan) { + this._updateSpan.finish(); + this._updateSpan = undefined; + } } // If a component is unmounted, we can say it is no longer on the screen. // This means we can finish the span representing the component render. public componentWillUnmount(): void { - const { name, includeRender = true } = this.props; + const {name, includeRender = true} = this.props; if (this._mountSpan && includeRender) { // If we were able to obtain the spanId of the mount activity, we should set the @@ -124,10 +138,13 @@ class Profiler extends React.Component { function withProfiler

>( WrappedComponent: React.ComponentType

, // We do not want to have `updateProps` given in options, it is instead filled through the HOC. - options?: Pick, Exclude>, + options?: Pick, Exclude> ): React.FC

{ const componentDisplayName = - (options && options.name) || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT; + (options && options.name) || + WrappedComponent.displayName || + WrappedComponent.name || + UNKNOWN_COMPONENT; const Wrapped: React.FC

= (props: P) => ( @@ -152,10 +169,10 @@ function withProfiler

>( */ function useProfiler( name: string, - options: { disabled?: boolean; hasRenderSpan?: boolean } = { + options: {disabled?: boolean; hasRenderSpan?: boolean} = { disabled: false, hasRenderSpan: true, - }, + } ): void { const [mountSpan] = React.useState(() => { if (options && options.disabled) { @@ -193,10 +210,12 @@ function useProfiler( }, []); } -export { withProfiler, Profiler, useProfiler }; +export {withProfiler, Profiler, useProfiler}; /** Grabs active transaction off scope */ -export function getActiveTransaction(hub: Hub = getCurrentHub()): T | undefined { +export function getActiveTransaction( + hub: Hub = getCurrentHub() +): T | undefined { if (hub) { const scope = hub.getScope(); if (scope) { diff --git a/packages/react/test/profiler.test.tsx b/packages/react/test/profiler.test.tsx index 2e458ed32871..388061a1956c 100644 --- a/packages/react/test/profiler.test.tsx +++ b/packages/react/test/profiler.test.tsx @@ -1,12 +1,12 @@ -import { SpanContext } from '@sentry/types'; -import { render } from '@testing-library/react'; -import { renderHook } from '@testing-library/react-hooks'; +import {SpanContext} from '@sentry/types'; +import {render} from '@testing-library/react'; +import {renderHook} from '@testing-library/react-hooks'; import * as React from 'react'; -import { REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP } from '../src/constants'; -import { UNKNOWN_COMPONENT, useProfiler, withProfiler } from '../src/profiler'; +import {REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP} from '../src/constants'; +import {UNKNOWN_COMPONENT, useProfiler, withProfiler} from '../src/profiler'; -const mockStartChild = jest.fn((spanArgs: SpanContext) => ({ ...spanArgs })); +const mockStartChild = jest.fn((spanArgs: SpanContext) => ({...spanArgs})); const mockFinish = jest.fn(); // @sent @@ -36,7 +36,8 @@ jest.mock('@sentry/browser', () => ({ beforeEach(() => { mockStartChild.mockClear(); - activeTransaction = new MockSpan({ op: 'pageload' }); + mockFinish.mockClear(); + activeTransaction = new MockSpan({op: 'pageload'}); }); describe('withProfiler', () => { @@ -50,7 +51,7 @@ describe('withProfiler', () => { it('sets a custom displayName', () => { const TestComponent = () =>

Hello World

; - const ProfiledComponent = withProfiler(TestComponent, { name: 'BestComponent' }); + const ProfiledComponent = withProfiler(TestComponent, {name: 'BestComponent'}); expect(ProfiledComponent.displayName).toBe('profiler(BestComponent)'); }); @@ -61,7 +62,7 @@ describe('withProfiler', () => { describe('mount span', () => { it('does not get created if Profiler is disabled', () => { - const ProfiledComponent = withProfiler(() =>

Testing

, { disabled: true }); + const ProfiledComponent = withProfiler(() =>

Testing

, {disabled: true}); expect(mockStartChild).toHaveBeenCalledTimes(0); render(); expect(mockStartChild).toHaveBeenCalledTimes(0); @@ -100,7 +101,9 @@ describe('withProfiler', () => { }); it('is not created if hasRenderSpan is false', () => { - const ProfiledComponent = withProfiler(() =>

Testing

, { includeRender: false }); + const ProfiledComponent = withProfiler(() =>

Testing

, { + includeRender: false, + }); expect(mockStartChild).toHaveBeenCalledTimes(0); const component = render(); @@ -112,28 +115,28 @@ describe('withProfiler', () => { describe('update span', () => { it('is created when component is updated', () => { - const ProfiledComponent = withProfiler((props: { num: number }) =>
{props.num}
); - const { rerender } = render(); + const ProfiledComponent = withProfiler((props: {num: number}) => ( +
{props.num}
+ )); + const {rerender} = render(); expect(mockStartChild).toHaveBeenCalledTimes(1); // Dispatch new props rerender(); expect(mockStartChild).toHaveBeenCalledTimes(2); expect(mockStartChild).toHaveBeenLastCalledWith({ - data: { changedProps: ['num'] }, + data: {changedProps: ['num']}, description: `<${UNKNOWN_COMPONENT}>`, - endTimestamp: expect.any(Number), op: REACT_UPDATE_OP, startTimestamp: expect.any(Number), }); - + expect(mockFinish).toHaveBeenCalledTimes(2); // New props yet again rerender(); expect(mockStartChild).toHaveBeenCalledTimes(3); expect(mockStartChild).toHaveBeenLastCalledWith({ - data: { changedProps: ['num'] }, + data: {changedProps: ['num']}, description: `<${UNKNOWN_COMPONENT}>`, - endTimestamp: expect.any(Number), op: REACT_UPDATE_OP, startTimestamp: expect.any(Number), }); @@ -141,13 +144,17 @@ describe('withProfiler', () => { // Should not create spans if props haven't changed rerender(); expect(mockStartChild).toHaveBeenCalledTimes(3); + expect(mockFinish).toHaveBeenCalledTimes(3); }); it('does not get created if hasUpdateSpan is false', () => { - const ProfiledComponent = withProfiler((props: { num: number }) =>
{props.num}
, { - includeUpdates: false, - }); - const { rerender } = render(); + const ProfiledComponent = withProfiler( + (props: {num: number}) =>
{props.num}
, + { + includeUpdates: false, + } + ); + const {rerender} = render(); expect(mockStartChild).toHaveBeenCalledTimes(1); // Dispatch new props @@ -160,7 +167,7 @@ describe('withProfiler', () => { describe('useProfiler()', () => { describe('mount span', () => { it('does not get created if Profiler is disabled', () => { - renderHook(() => useProfiler('Example', { disabled: true })); + renderHook(() => useProfiler('Example', {disabled: true})); expect(mockStartChild).toHaveBeenCalledTimes(0); }); @@ -177,7 +184,7 @@ describe('useProfiler()', () => { describe('render span', () => { it('does not get created when hasRenderSpan is false', () => { - const component = renderHook(() => useProfiler('Example', { hasRenderSpan: false })); + const component = renderHook(() => useProfiler('Example', {hasRenderSpan: false})); expect(mockStartChild).toHaveBeenCalledTimes(1); component.unmount(); expect(mockStartChild).toHaveBeenCalledTimes(1); @@ -193,7 +200,7 @@ describe('useProfiler()', () => { expect.objectContaining({ description: '', op: REACT_RENDER_OP, - }), + }) ); }); }); From 3c4d3c0bbe2dece32c15e81af1f8ff7a17d2824c Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Fri, 5 Aug 2022 15:33:58 -0400 Subject: [PATCH 2/7] Use Prettier config from workspace --- .vscode/settings.json | 1 + packages/react/src/profiler.tsx | 40 +++++++++++++-------------------- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 82e3bc618c58..6d9b7fd82bd5 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -21,4 +21,5 @@ "yaml.schemas": { "https://json.schemastore.org/github-workflow.json": ".github/workflows/**.yml" }, + "prettier.configPath": "/Users/ash/code/sentry-javascript/.prettierrc.json" } diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx index 2a8b1bdc0b83..402cb0fd8aab 100644 --- a/packages/react/src/profiler.tsx +++ b/packages/react/src/profiler.tsx @@ -1,12 +1,12 @@ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import {getCurrentHub, Hub} from '@sentry/browser'; -import {Span, Transaction} from '@sentry/types'; -import {timestampWithMs} from '@sentry/utils'; +import { getCurrentHub, Hub } from '@sentry/browser'; +import { Span, Transaction } from '@sentry/types'; +import { timestampWithMs } from '@sentry/utils'; import hoistNonReactStatics from 'hoist-non-react-statics'; import * as React from 'react'; -import {REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP} from './constants'; +import { REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP } from './constants'; export const UNKNOWN_COMPONENT = 'unknown'; @@ -21,7 +21,7 @@ export type ProfilerProps = { // If component updates should be displayed as spans. True by default. includeUpdates?: boolean; // props given to component being profiled. - updateProps: {[key: string]: unknown}; + updateProps: { [key: string]: unknown }; }; /** @@ -45,7 +45,7 @@ class Profiler extends React.Component { public constructor(props: ProfilerProps) { super(props); - const {name, disabled = false} = this.props; + const { name, disabled = false } = this.props; if (disabled) { return; @@ -67,16 +67,11 @@ class Profiler extends React.Component { } } - public shouldComponentUpdate({ - updateProps, - includeUpdates = true, - }: ProfilerProps): boolean { + public shouldComponentUpdate({ updateProps, includeUpdates = true }: ProfilerProps): boolean { if (includeUpdates && this._mountSpan && updateProps !== this.props.updateProps) { // See what props haved changed between the previous props, and the current props. This is // set as data on the span. We just store the prop keys as the values could be potenially very large. - const changedProps = Object.keys(updateProps).filter( - k => updateProps[k] !== this.props.updateProps[k] - ); + const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]); if (changedProps.length > 0) { // The update span is a point in time span with 0 duration, just signifying that the component // has been updated. @@ -108,7 +103,7 @@ class Profiler extends React.Component { // If a component is unmounted, we can say it is no longer on the screen. // This means we can finish the span representing the component render. public componentWillUnmount(): void { - const {name, includeRender = true} = this.props; + const { name, includeRender = true } = this.props; if (this._mountSpan && includeRender) { // If we were able to obtain the spanId of the mount activity, we should set the @@ -138,13 +133,10 @@ class Profiler extends React.Component { function withProfiler

>( WrappedComponent: React.ComponentType

, // We do not want to have `updateProps` given in options, it is instead filled through the HOC. - options?: Pick, Exclude> + options?: Pick, Exclude>, ): React.FC

{ const componentDisplayName = - (options && options.name) || - WrappedComponent.displayName || - WrappedComponent.name || - UNKNOWN_COMPONENT; + (options && options.name) || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT; const Wrapped: React.FC

= (props: P) => ( @@ -169,10 +161,10 @@ function withProfiler

>( */ function useProfiler( name: string, - options: {disabled?: boolean; hasRenderSpan?: boolean} = { + options: { disabled?: boolean; hasRenderSpan?: boolean } = { disabled: false, hasRenderSpan: true, - } + }, ): void { const [mountSpan] = React.useState(() => { if (options && options.disabled) { @@ -210,12 +202,10 @@ function useProfiler( }, []); } -export {withProfiler, Profiler, useProfiler}; +export { withProfiler, Profiler, useProfiler }; /** Grabs active transaction off scope */ -export function getActiveTransaction( - hub: Hub = getCurrentHub() -): T | undefined { +export function getActiveTransaction(hub: Hub = getCurrentHub()): T | undefined { if (hub) { const scope = hub.getScope(); if (scope) { From 1a1555b4a9958a9774ecd9d500e8231267bdcb9a Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Fri, 5 Aug 2022 15:38:02 -0400 Subject: [PATCH 3/7] Remove changes to vscode settings --- .vscode/settings.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 6d9b7fd82bd5..6016b2dd51bc 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -20,6 +20,5 @@ }, "yaml.schemas": { "https://json.schemastore.org/github-workflow.json": ".github/workflows/**.yml" - }, - "prettier.configPath": "/Users/ash/code/sentry-javascript/.prettierrc.json" + } } From ac107fc97f33a53207ebc8d37d292a3f627c4b52 Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Fri, 5 Aug 2022 15:43:06 -0400 Subject: [PATCH 4/7] Move comments --- packages/react/src/profiler.tsx | 46 ++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx index 402cb0fd8aab..ee85ddabcbd0 100644 --- a/packages/react/src/profiler.tsx +++ b/packages/react/src/profiler.tsx @@ -1,12 +1,12 @@ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { getCurrentHub, Hub } from '@sentry/browser'; -import { Span, Transaction } from '@sentry/types'; -import { timestampWithMs } from '@sentry/utils'; +import {getCurrentHub, Hub} from '@sentry/browser'; +import {Span, Transaction} from '@sentry/types'; +import {timestampWithMs} from '@sentry/utils'; import hoistNonReactStatics from 'hoist-non-react-statics'; import * as React from 'react'; -import { REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP } from './constants'; +import {REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP} from './constants'; export const UNKNOWN_COMPONENT = 'unknown'; @@ -21,7 +21,7 @@ export type ProfilerProps = { // If component updates should be displayed as spans. True by default. includeUpdates?: boolean; // props given to component being profiled. - updateProps: { [key: string]: unknown }; + updateProps: {[key: string]: unknown}; }; /** @@ -45,7 +45,7 @@ class Profiler extends React.Component { public constructor(props: ProfilerProps) { super(props); - const { name, disabled = false } = this.props; + const {name, disabled = false} = this.props; if (disabled) { return; @@ -67,11 +67,19 @@ class Profiler extends React.Component { } } - public shouldComponentUpdate({ updateProps, includeUpdates = true }: ProfilerProps): boolean { + public shouldComponentUpdate({ + updateProps, + includeUpdates = true, + }: ProfilerProps): boolean { + // Only generate an update span if includeUpdates is true, if there is a valid mountSpan, + // and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive. + // We are just trying to give baseline clues for further investigation. if (includeUpdates && this._mountSpan && updateProps !== this.props.updateProps) { // See what props haved changed between the previous props, and the current props. This is // set as data on the span. We just store the prop keys as the values could be potenially very large. - const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]); + const changedProps = Object.keys(updateProps).filter( + k => updateProps[k] !== this.props.updateProps[k] + ); if (changedProps.length > 0) { // The update span is a point in time span with 0 duration, just signifying that the component // has been updated. @@ -91,9 +99,6 @@ class Profiler extends React.Component { } public componentDidUpdate(): void { - // Only generate an update span if hasUpdateSpan is true, if there is a valid mountSpan, - // and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive. - // We are just trying to give baseline clues for further investigation. if (this._updateSpan) { this._updateSpan.finish(); this._updateSpan = undefined; @@ -103,7 +108,7 @@ class Profiler extends React.Component { // If a component is unmounted, we can say it is no longer on the screen. // This means we can finish the span representing the component render. public componentWillUnmount(): void { - const { name, includeRender = true } = this.props; + const {name, includeRender = true} = this.props; if (this._mountSpan && includeRender) { // If we were able to obtain the spanId of the mount activity, we should set the @@ -133,10 +138,13 @@ class Profiler extends React.Component { function withProfiler

>( WrappedComponent: React.ComponentType

, // We do not want to have `updateProps` given in options, it is instead filled through the HOC. - options?: Pick, Exclude>, + options?: Pick, Exclude> ): React.FC

{ const componentDisplayName = - (options && options.name) || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT; + (options && options.name) || + WrappedComponent.displayName || + WrappedComponent.name || + UNKNOWN_COMPONENT; const Wrapped: React.FC

= (props: P) => ( @@ -161,10 +169,10 @@ function withProfiler

>( */ function useProfiler( name: string, - options: { disabled?: boolean; hasRenderSpan?: boolean } = { + options: {disabled?: boolean; hasRenderSpan?: boolean} = { disabled: false, hasRenderSpan: true, - }, + } ): void { const [mountSpan] = React.useState(() => { if (options && options.disabled) { @@ -202,10 +210,12 @@ function useProfiler( }, []); } -export { withProfiler, Profiler, useProfiler }; +export {withProfiler, Profiler, useProfiler}; /** Grabs active transaction off scope */ -export function getActiveTransaction(hub: Hub = getCurrentHub()): T | undefined { +export function getActiveTransaction( + hub: Hub = getCurrentHub() +): T | undefined { if (hub) { const scope = hub.getScope(); if (scope) { From fb96942a67831c7eabbef4a67d4ca2ce08affb12 Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Fri, 5 Aug 2022 15:46:43 -0400 Subject: [PATCH 5/7] Remove VSCode settings changes --- .vscode/settings.json | 2 +- packages/react/src/profiler.tsx | 40 +++++++++++++-------------------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 6016b2dd51bc..82e3bc618c58 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -20,5 +20,5 @@ }, "yaml.schemas": { "https://json.schemastore.org/github-workflow.json": ".github/workflows/**.yml" - } + }, } diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx index ee85ddabcbd0..470fd81928e3 100644 --- a/packages/react/src/profiler.tsx +++ b/packages/react/src/profiler.tsx @@ -1,12 +1,12 @@ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ /* eslint-disable @typescript-eslint/no-explicit-any */ -import {getCurrentHub, Hub} from '@sentry/browser'; -import {Span, Transaction} from '@sentry/types'; -import {timestampWithMs} from '@sentry/utils'; +import { getCurrentHub, Hub } from '@sentry/browser'; +import { Span, Transaction } from '@sentry/types'; +import { timestampWithMs } from '@sentry/utils'; import hoistNonReactStatics from 'hoist-non-react-statics'; import * as React from 'react'; -import {REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP} from './constants'; +import { REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP } from './constants'; export const UNKNOWN_COMPONENT = 'unknown'; @@ -21,7 +21,7 @@ export type ProfilerProps = { // If component updates should be displayed as spans. True by default. includeUpdates?: boolean; // props given to component being profiled. - updateProps: {[key: string]: unknown}; + updateProps: { [key: string]: unknown }; }; /** @@ -45,7 +45,7 @@ class Profiler extends React.Component { public constructor(props: ProfilerProps) { super(props); - const {name, disabled = false} = this.props; + const { name, disabled = false } = this.props; if (disabled) { return; @@ -67,19 +67,14 @@ class Profiler extends React.Component { } } - public shouldComponentUpdate({ - updateProps, - includeUpdates = true, - }: ProfilerProps): boolean { + public shouldComponentUpdate({ updateProps, includeUpdates = true }: ProfilerProps): boolean { // Only generate an update span if includeUpdates is true, if there is a valid mountSpan, // and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive. // We are just trying to give baseline clues for further investigation. if (includeUpdates && this._mountSpan && updateProps !== this.props.updateProps) { // See what props haved changed between the previous props, and the current props. This is // set as data on the span. We just store the prop keys as the values could be potenially very large. - const changedProps = Object.keys(updateProps).filter( - k => updateProps[k] !== this.props.updateProps[k] - ); + const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]); if (changedProps.length > 0) { // The update span is a point in time span with 0 duration, just signifying that the component // has been updated. @@ -108,7 +103,7 @@ class Profiler extends React.Component { // If a component is unmounted, we can say it is no longer on the screen. // This means we can finish the span representing the component render. public componentWillUnmount(): void { - const {name, includeRender = true} = this.props; + const { name, includeRender = true } = this.props; if (this._mountSpan && includeRender) { // If we were able to obtain the spanId of the mount activity, we should set the @@ -138,13 +133,10 @@ class Profiler extends React.Component { function withProfiler

>( WrappedComponent: React.ComponentType

, // We do not want to have `updateProps` given in options, it is instead filled through the HOC. - options?: Pick, Exclude> + options?: Pick, Exclude>, ): React.FC

{ const componentDisplayName = - (options && options.name) || - WrappedComponent.displayName || - WrappedComponent.name || - UNKNOWN_COMPONENT; + (options && options.name) || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT; const Wrapped: React.FC

= (props: P) => ( @@ -169,10 +161,10 @@ function withProfiler

>( */ function useProfiler( name: string, - options: {disabled?: boolean; hasRenderSpan?: boolean} = { + options: { disabled?: boolean; hasRenderSpan?: boolean } = { disabled: false, hasRenderSpan: true, - } + }, ): void { const [mountSpan] = React.useState(() => { if (options && options.disabled) { @@ -210,12 +202,10 @@ function useProfiler( }, []); } -export {withProfiler, Profiler, useProfiler}; +export { withProfiler, Profiler, useProfiler }; /** Grabs active transaction off scope */ -export function getActiveTransaction( - hub: Hub = getCurrentHub() -): T | undefined { +export function getActiveTransaction(hub: Hub = getCurrentHub()): T | undefined { if (hub) { const scope = hub.getScope(); if (scope) { From 0608e162c22230e99f55beda7cd755c5ebf626a4 Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Fri, 5 Aug 2022 15:47:50 -0400 Subject: [PATCH 6/7] Remove Prettier changes in test file --- packages/react/test/profiler.test.tsx | 45 ++++++++++++--------------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/packages/react/test/profiler.test.tsx b/packages/react/test/profiler.test.tsx index 388061a1956c..29d8152c68f4 100644 --- a/packages/react/test/profiler.test.tsx +++ b/packages/react/test/profiler.test.tsx @@ -1,12 +1,12 @@ -import {SpanContext} from '@sentry/types'; -import {render} from '@testing-library/react'; -import {renderHook} from '@testing-library/react-hooks'; +import { SpanContext } from '@sentry/types'; +import { render } from '@testing-library/react'; +import { renderHook } from '@testing-library/react-hooks'; import * as React from 'react'; -import {REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP} from '../src/constants'; -import {UNKNOWN_COMPONENT, useProfiler, withProfiler} from '../src/profiler'; +import { REACT_MOUNT_OP, REACT_RENDER_OP, REACT_UPDATE_OP } from '../src/constants'; +import { UNKNOWN_COMPONENT, useProfiler, withProfiler } from '../src/profiler'; -const mockStartChild = jest.fn((spanArgs: SpanContext) => ({...spanArgs})); +const mockStartChild = jest.fn((spanArgs: SpanContext) => ({ ...spanArgs })); const mockFinish = jest.fn(); // @sent @@ -37,7 +37,7 @@ jest.mock('@sentry/browser', () => ({ beforeEach(() => { mockStartChild.mockClear(); mockFinish.mockClear(); - activeTransaction = new MockSpan({op: 'pageload'}); + activeTransaction = new MockSpan({ op: 'pageload' }); }); describe('withProfiler', () => { @@ -51,7 +51,7 @@ describe('withProfiler', () => { it('sets a custom displayName', () => { const TestComponent = () =>

Hello World

; - const ProfiledComponent = withProfiler(TestComponent, {name: 'BestComponent'}); + const ProfiledComponent = withProfiler(TestComponent, { name: 'BestComponent' }); expect(ProfiledComponent.displayName).toBe('profiler(BestComponent)'); }); @@ -62,7 +62,7 @@ describe('withProfiler', () => { describe('mount span', () => { it('does not get created if Profiler is disabled', () => { - const ProfiledComponent = withProfiler(() =>

Testing

, {disabled: true}); + const ProfiledComponent = withProfiler(() =>

Testing

, { disabled: true }); expect(mockStartChild).toHaveBeenCalledTimes(0); render(); expect(mockStartChild).toHaveBeenCalledTimes(0); @@ -115,17 +115,15 @@ describe('withProfiler', () => { describe('update span', () => { it('is created when component is updated', () => { - const ProfiledComponent = withProfiler((props: {num: number}) => ( -
{props.num}
- )); - const {rerender} = render(); + const ProfiledComponent = withProfiler((props: { num: number }) =>
{props.num}
); + const { rerender } = render(); expect(mockStartChild).toHaveBeenCalledTimes(1); // Dispatch new props rerender(); expect(mockStartChild).toHaveBeenCalledTimes(2); expect(mockStartChild).toHaveBeenLastCalledWith({ - data: {changedProps: ['num']}, + data: { changedProps: ['num'] }, description: `<${UNKNOWN_COMPONENT}>`, op: REACT_UPDATE_OP, startTimestamp: expect.any(Number), @@ -135,7 +133,7 @@ describe('withProfiler', () => { rerender(); expect(mockStartChild).toHaveBeenCalledTimes(3); expect(mockStartChild).toHaveBeenLastCalledWith({ - data: {changedProps: ['num']}, + data: { changedProps: ['num'] }, description: `<${UNKNOWN_COMPONENT}>`, op: REACT_UPDATE_OP, startTimestamp: expect.any(Number), @@ -148,13 +146,10 @@ describe('withProfiler', () => { }); it('does not get created if hasUpdateSpan is false', () => { - const ProfiledComponent = withProfiler( - (props: {num: number}) =>
{props.num}
, - { - includeUpdates: false, - } - ); - const {rerender} = render(); + const ProfiledComponent = withProfiler((props: { num: number }) =>
{props.num}
, { + includeUpdates: false, + }); + const { rerender } = render(); expect(mockStartChild).toHaveBeenCalledTimes(1); // Dispatch new props @@ -167,7 +162,7 @@ describe('withProfiler', () => { describe('useProfiler()', () => { describe('mount span', () => { it('does not get created if Profiler is disabled', () => { - renderHook(() => useProfiler('Example', {disabled: true})); + renderHook(() => useProfiler('Example', { disabled: true })); expect(mockStartChild).toHaveBeenCalledTimes(0); }); @@ -184,7 +179,7 @@ describe('useProfiler()', () => { describe('render span', () => { it('does not get created when hasRenderSpan is false', () => { - const component = renderHook(() => useProfiler('Example', {hasRenderSpan: false})); + const component = renderHook(() => useProfiler('Example', { hasRenderSpan: false })); expect(mockStartChild).toHaveBeenCalledTimes(1); component.unmount(); expect(mockStartChild).toHaveBeenCalledTimes(1); @@ -200,7 +195,7 @@ describe('useProfiler()', () => { expect.objectContaining({ description: '', op: REACT_RENDER_OP, - }) + }), ); }); }); From 7cc89842a7ad5b1a1f6cf80aa7cbe3c64a1d5896 Mon Sep 17 00:00:00 2001 From: Ash Anand Date: Tue, 9 Aug 2022 16:34:20 -0400 Subject: [PATCH 7/7] Add docstring and additional asserts on mockFinish --- packages/react/src/profiler.tsx | 5 +++-- packages/react/test/profiler.test.tsx | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx index 470fd81928e3..ecf7608dac3a 100644 --- a/packages/react/src/profiler.tsx +++ b/packages/react/src/profiler.tsx @@ -34,6 +34,9 @@ class Profiler extends React.Component { * Made protected for the React Native SDK to access */ protected _mountSpan: Span | undefined = undefined; + /** + * The span that represents the duration of time between shouldComponentUpdate and componentDidUpdate + */ protected _updateSpan: Span | undefined = undefined; // eslint-disable-next-line @typescript-eslint/member-ordering @@ -76,8 +79,6 @@ class Profiler extends React.Component { // set as data on the span. We just store the prop keys as the values could be potenially very large. const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]); if (changedProps.length > 0) { - // The update span is a point in time span with 0 duration, just signifying that the component - // has been updated. const now = timestampWithMs(); this._updateSpan = this._mountSpan.startChild({ data: { diff --git a/packages/react/test/profiler.test.tsx b/packages/react/test/profiler.test.tsx index 29d8152c68f4..96988f24b44b 100644 --- a/packages/react/test/profiler.test.tsx +++ b/packages/react/test/profiler.test.tsx @@ -118,6 +118,7 @@ describe('withProfiler', () => { const ProfiledComponent = withProfiler((props: { num: number }) =>
{props.num}
); const { rerender } = render(); expect(mockStartChild).toHaveBeenCalledTimes(1); + expect(mockFinish).toHaveBeenCalledTimes(1); // Dispatch new props rerender(); @@ -138,6 +139,7 @@ describe('withProfiler', () => { op: REACT_UPDATE_OP, startTimestamp: expect.any(Number), }); + expect(mockFinish).toHaveBeenCalledTimes(3); // Should not create spans if props haven't changed rerender();