Skip to content

Commit 092367d

Browse files
authored
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
1 parent e8093c2 commit 092367d

File tree

7 files changed

+437
-21
lines changed

7 files changed

+437
-21
lines changed

packages/react/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
"@types/hoist-non-react-statics": "^3.3.1",
3333
"@types/react": "^16.9.35",
3434
"jest": "^24.7.1",
35+
"jsdom": "^16.2.2",
3536
"npm-run-all": "^4.1.2",
3637
"prettier": "^1.17.0",
3738
"prettier-check": "^2.0.0",

packages/react/src/errorboundary.tsx

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,31 @@ export type FallbackRender = (fallback: {
1111
}) => React.ReactNode;
1212

1313
export type ErrorBoundaryProps = {
14+
/** If a Sentry report dialog should be rendered on error */
1415
showDialog?: boolean;
16+
/**
17+
* Options to be passed into the Sentry report dialog.
18+
* No-op if {@link showDialog} is false.
19+
*/
1520
dialogOptions?: Sentry.ReportDialogOptions;
16-
// tslint:disable-next-line: no-null-undefined-union
21+
// tslint:disable no-null-undefined-union
22+
/**
23+
* A fallback component that gets rendered when the error boundary encounters an error.
24+
*
25+
* Can either provide a React Component, or a function that returns React Component as
26+
* a valid fallback prop. If a function is provided, the function will be called with
27+
* the error, the component stack, and an function that resets the error boundary on error.
28+
*
29+
*/
1730
fallback?: React.ReactNode | FallbackRender;
31+
// tslint:enable no-null-undefined-union
32+
/** Called with the error boundary encounters an error */
1833
onError?(error: Error, componentStack: string): void;
34+
/** Called on componentDidMount() */
1935
onMount?(): void;
36+
/** Called if resetError() is called from the fallback render props function */
2037
onReset?(error: Error | null, componentStack: string | null): void;
38+
/** Called on componentWillUnmount() */
2139
onUnmount?(error: Error | null, componentStack: string | null): void;
2240
};
2341

@@ -31,17 +49,21 @@ const INITIAL_STATE = {
3149
error: null,
3250
};
3351

52+
/**
53+
* A ErrorBoundary component that logs errors to Sentry.
54+
* Requires React >= 16
55+
*/
3456
class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundaryState> {
3557
public state: ErrorBoundaryState = INITIAL_STATE;
3658

3759
public componentDidCatch(error: Error, { componentStack }: React.ErrorInfo): void {
38-
Sentry.captureException(error, { contexts: { react: { componentStack } } });
60+
const eventId = Sentry.captureException(error, { contexts: { react: { componentStack } } });
3961
const { onError, showDialog, dialogOptions } = this.props;
4062
if (onError) {
4163
onError(error, componentStack);
4264
}
4365
if (showDialog) {
44-
Sentry.showReportDialog(dialogOptions);
66+
Sentry.showReportDialog({ ...dialogOptions, eventId });
4567
}
4668

4769
// componentDidCatch is used over getDerivedStateFromError

packages/react/src/index.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,27 @@
1+
import { addGlobalEventProcessor, SDK_VERSION } from '@sentry/browser';
2+
3+
function createReactEventProcessor(): void {
4+
addGlobalEventProcessor(event => {
5+
event.sdk = {
6+
...event.sdk,
7+
name: 'sentry.javascript.react',
8+
packages: [
9+
...((event.sdk && event.sdk.packages) || []),
10+
{
11+
name: 'npm:@sentry/react',
12+
version: SDK_VERSION,
13+
},
14+
],
15+
version: SDK_VERSION,
16+
};
17+
18+
return event;
19+
});
20+
}
21+
122
export * from '@sentry/browser';
223

324
export { Profiler, withProfiler, useProfiler } from './profiler';
425
export { ErrorBoundary, withErrorBoundary } from './errorboundary';
26+
27+
createReactEventProcessor();

packages/react/src/profiler.tsx

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ function afterNextFrame(callback: Function): void {
3939
timeout = window.setTimeout(done, 100);
4040
}
4141

42+
/**
43+
* getInitActivity pushes activity based on React component mount
44+
* @param name displayName of component that started activity
45+
*/
4246
const getInitActivity = (name: string): number | null => {
4347
const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
4448

@@ -68,10 +72,13 @@ class Profiler extends React.Component<ProfilerProps> {
6872
this.activity = getInitActivity(this.props.name);
6973
}
7074

75+
// If a component mounted, we can finish the mount activity.
7176
public componentDidMount(): void {
7277
afterNextFrame(this.finishProfile);
7378
}
7479

80+
// Sometimes a component will unmount first, so we make
81+
// sure to also finish the mount activity here.
7582
public componentWillUnmount(): void {
7683
afterNextFrame(this.finishProfile);
7784
}
@@ -94,8 +101,15 @@ class Profiler extends React.Component<ProfilerProps> {
94101
}
95102
}
96103

97-
function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>): React.FC<P> {
98-
const componentDisplayName = WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
104+
/**
105+
* withProfiler is a higher order component that wraps a
106+
* component in a {@link Profiler} component.
107+
*
108+
* @param WrappedComponent component that is wrapped by Profiler
109+
* @param name displayName of component being profiled
110+
*/
111+
function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>, name?: string): React.FC<P> {
112+
const componentDisplayName = name || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
99113

100114
const Wrapped: React.FC<P> = (props: P) => (
101115
<Profiler name={componentDisplayName}>
@@ -119,7 +133,7 @@ function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>
119133
* @param name displayName of component being profiled
120134
*/
121135
function useProfiler(name: string): void {
122-
const activity = getInitActivity(name);
136+
const [activity] = React.useState(() => getInitActivity(name));
123137

124138
React.useEffect(() => {
125139
afterNextFrame(() => {

packages/react/test/errorboundary.test.tsx

Lines changed: 48 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
11
import { fireEvent, render, screen } from '@testing-library/react';
22
import * as React from 'react';
33

4-
import { ErrorBoundary, ErrorBoundaryProps } from '../src/errorboundary';
4+
import { ErrorBoundary, ErrorBoundaryProps, UNKNOWN_COMPONENT, withErrorBoundary } from '../src/errorboundary';
55

66
const mockCaptureException = jest.fn();
77
const mockShowReportDialog = jest.fn();
8+
const EVENT_ID = 'test-id-123';
89

910
jest.mock('@sentry/browser', () => ({
1011
captureException: (err: any, ctx: any) => {
1112
mockCaptureException(err, ctx);
13+
return EVENT_ID;
1214
},
1315
showReportDialog: (options: any) => {
1416
mockShowReportDialog(options);
@@ -18,7 +20,15 @@ jest.mock('@sentry/browser', () => ({
1820
const TestApp: React.FC<ErrorBoundaryProps> = ({ children, ...props }) => {
1921
const [isError, setError] = React.useState(false);
2022
return (
21-
<ErrorBoundary {...props}>
23+
<ErrorBoundary
24+
{...props}
25+
onReset={(err: Error, stack: string) => {
26+
setError(false);
27+
if (props.onReset) {
28+
props.onReset(err, stack);
29+
}
30+
}}
31+
>
2232
{isError ? <Bam /> : children}
2333
<button
2434
data-testid="errorBtn"
@@ -34,6 +44,20 @@ function Bam(): JSX.Element {
3444
throw new Error('boom');
3545
}
3646

47+
describe('withErrorBoundary', () => {
48+
it('sets displayName properly', () => {
49+
const TestComponent = () => <h1>Hello World</h1>;
50+
51+
const Component = withErrorBoundary(TestComponent, { fallback: <h1>fallback</h1> });
52+
expect(Component.displayName).toBe('errorBoundary(TestComponent)');
53+
});
54+
55+
it('defaults to an unknown displayName', () => {
56+
const Component = withErrorBoundary(() => <h1>Hello World</h1>, { fallback: <h1>fallback</h1> });
57+
expect(Component.displayName).toBe(`errorBoundary(${UNKNOWN_COMPONENT})`);
58+
});
59+
});
60+
3761
describe('ErrorBoundary', () => {
3862
jest.spyOn(console, 'error').mockImplementation();
3963

@@ -44,7 +68,6 @@ describe('ErrorBoundary', () => {
4468

4569
it('renders null if not given a valid `fallback` prop', () => {
4670
const { container } = render(
47-
// @ts-ignore
4871
<ErrorBoundary fallback={new Error('true')}>
4972
<Bam />
5073
</ErrorBoundary>,
@@ -55,7 +78,6 @@ describe('ErrorBoundary', () => {
5578

5679
it('renders a fallback on error', () => {
5780
const { container } = render(
58-
// @ts-ignore
5981
<ErrorBoundary fallback={<h1>Error Component</h1>}>
6082
<Bam />
6183
</ErrorBoundary>,
@@ -186,12 +208,30 @@ describe('ErrorBoundary', () => {
186208
fireEvent.click(btn);
187209

188210
expect(mockShowReportDialog).toHaveBeenCalledTimes(1);
189-
expect(mockShowReportDialog).toHaveBeenCalledWith(options);
211+
expect(mockShowReportDialog).toHaveBeenCalledWith({ ...options, eventId: EVENT_ID });
190212
});
191213

192-
it('resets to initial state when reset', () => {
193-
const mockOnReset = jest.fn();
214+
it('resets to initial state when reset', async () => {
194215
const { container } = render(
216+
<TestApp fallback={({ resetError }) => <button data-testid="reset" onClick={resetError} />}>
217+
<h1>children</h1>
218+
</TestApp>,
219+
);
220+
221+
expect(container.innerHTML).toContain('<h1>children</h1>');
222+
const btn = screen.getByTestId('errorBtn');
223+
fireEvent.click(btn);
224+
expect(container.innerHTML).toContain('<button data-testid="reset">');
225+
226+
const reset = screen.getByTestId('reset');
227+
fireEvent.click(reset);
228+
229+
expect(container.innerHTML).toContain('<h1>children</h1>');
230+
});
231+
232+
it('calls `onReset()` when reset', () => {
233+
const mockOnReset = jest.fn();
234+
render(
195235
<TestApp
196236
onReset={mockOnReset}
197237
fallback={({ resetError }) => <button data-testid="reset" onClick={resetError} />}
@@ -200,17 +240,14 @@ describe('ErrorBoundary', () => {
200240
</TestApp>,
201241
);
202242

203-
expect(container.innerHTML).toContain('<h1>children</h1>');
204243
expect(mockOnReset).toHaveBeenCalledTimes(0);
205-
206244
const btn = screen.getByTestId('errorBtn');
207245
fireEvent.click(btn);
208-
209-
expect(container.innerHTML).toContain('<button data-testid="reset">');
210246
expect(mockOnReset).toHaveBeenCalledTimes(0);
211247

212248
const reset = screen.getByTestId('reset');
213249
fireEvent.click(reset);
250+
214251
expect(mockOnReset).toHaveBeenCalledTimes(1);
215252
expect(mockOnReset).toHaveBeenCalledWith(expect.any(Error), expect.any(String));
216253
});

packages/react/test/profiler.test.tsx

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,17 @@ import { UNKNOWN_COMPONENT, useProfiler, withProfiler } from '../src/profiler';
66

77
const mockPushActivity = jest.fn().mockReturnValue(1);
88
const mockPopActivity = jest.fn();
9+
const mockLoggerWarn = jest.fn();
10+
11+
let integrationIsNull = false;
12+
13+
jest.mock('@sentry/utils', () => ({
14+
logger: {
15+
warn: (message: string) => {
16+
mockLoggerWarn(message);
17+
},
18+
},
19+
}));
920

1021
jest.mock('@sentry/browser', () => ({
1122
getCurrentHub: () => ({
@@ -20,7 +31,11 @@ jest.mock('@sentry/browser', () => ({
2031
public static popActivity: () => void = mockPopActivity;
2132
}
2233

23-
return new MockIntegration('test');
34+
if (!integrationIsNull) {
35+
return new MockIntegration('test');
36+
}
37+
38+
return null;
2439
},
2540
}),
2641
}));
@@ -30,6 +45,8 @@ describe('withProfiler', () => {
3045
jest.useFakeTimers();
3146
mockPushActivity.mockClear();
3247
mockPopActivity.mockClear();
48+
mockLoggerWarn.mockClear();
49+
integrationIsNull = false;
3350
});
3451

3552
it('sets displayName properly', () => {
@@ -39,6 +56,18 @@ describe('withProfiler', () => {
3956
expect(ProfiledComponent.displayName).toBe('profiler(TestComponent)');
4057
});
4158

59+
it('sets a custom displayName', () => {
60+
const TestComponent = () => <h1>Hello World</h1>;
61+
62+
const ProfiledComponent = withProfiler(TestComponent, 'BestComponent');
63+
expect(ProfiledComponent.displayName).toBe('profiler(BestComponent)');
64+
});
65+
66+
it('defaults to an unknown displayName', () => {
67+
const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>);
68+
expect(ProfiledComponent.displayName).toBe(`profiler(${UNKNOWN_COMPONENT})`);
69+
});
70+
4271
it('popActivity() is called when unmounted', () => {
4372
const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>);
4473

@@ -63,13 +92,32 @@ describe('withProfiler', () => {
6392
op: 'react',
6493
});
6594
});
95+
96+
it('does not start an activity when integration is disabled', () => {
97+
integrationIsNull = true;
98+
const ProfiledComponent = withProfiler(() => <h1>Hello World</h1>);
99+
100+
expect(mockPushActivity).toHaveBeenCalledTimes(0);
101+
expect(mockLoggerWarn).toHaveBeenCalledTimes(0);
102+
103+
const profiler = render(<ProfiledComponent />);
104+
expect(mockPopActivity).toHaveBeenCalledTimes(0);
105+
expect(mockPushActivity).toHaveBeenCalledTimes(0);
106+
107+
expect(mockLoggerWarn).toHaveBeenCalledTimes(1);
108+
109+
profiler.unmount();
110+
expect(mockPopActivity).toHaveBeenCalledTimes(0);
111+
});
66112
});
67113

68114
describe('useProfiler()', () => {
69115
beforeEach(() => {
70116
jest.useFakeTimers();
71117
mockPushActivity.mockClear();
72118
mockPopActivity.mockClear();
119+
mockLoggerWarn.mockClear();
120+
integrationIsNull = false;
73121
});
74122

75123
it('popActivity() is called when unmounted', () => {
@@ -95,4 +143,20 @@ describe('useProfiler()', () => {
95143
op: 'react',
96144
});
97145
});
146+
147+
it('does not start an activity when integration is disabled', () => {
148+
integrationIsNull = true;
149+
expect(mockPushActivity).toHaveBeenCalledTimes(0);
150+
expect(mockLoggerWarn).toHaveBeenCalledTimes(0);
151+
152+
// tslint:disable-next-line: no-void-expression
153+
const profiler = renderHook(() => useProfiler('Example'));
154+
expect(mockPopActivity).toHaveBeenCalledTimes(0);
155+
expect(mockPushActivity).toHaveBeenCalledTimes(0);
156+
157+
expect(mockLoggerWarn).toHaveBeenCalledTimes(1);
158+
159+
profiler.unmount();
160+
expect(mockPopActivity).toHaveBeenCalledTimes(0);
161+
});
98162
});

0 commit comments

Comments
 (0)