-
Notifications
You must be signed in to change notification settings - Fork 4
Adding React Backtrace Client, ErrorBoundary, and tests #38
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
Changes from all commits
3bdddc1
12642ba
b03562d
f9c219a
228af9a
f4d07a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| /** @type {import('ts-jest').JestConfigWithTsJest} */ | ||
| module.exports = { | ||
| preset: 'ts-jest', | ||
| testEnvironment: 'jsdom', | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { | ||
| BacktraceAttributeProvider, | ||
| BacktraceCoreClient, | ||
| BacktraceRequestHandler, | ||
| BacktraceStackTraceConverter, | ||
| } from '@backtrace/sdk-core'; | ||
| import { AGENT } from '@backtrace/browser'; | ||
| import { BacktraceConfiguration } from '@backtrace/browser'; | ||
| import { BacktraceClientBuilder } from '@backtrace/browser'; | ||
|
|
||
| export class BacktraceClient extends BacktraceCoreClient { | ||
| private static _instance?: BacktraceClient; | ||
| constructor( | ||
| options: BacktraceConfiguration, | ||
| handler: BacktraceRequestHandler, | ||
| attributeProviders: BacktraceAttributeProvider[], | ||
| stackTraceConverter: BacktraceStackTraceConverter, | ||
| ) { | ||
| super(options, AGENT, handler, attributeProviders, stackTraceConverter); | ||
| } | ||
|
|
||
| public static builder(options: BacktraceConfiguration): BacktraceClientBuilder { | ||
| return new BacktraceClientBuilder(options); | ||
| } | ||
|
|
||
| public static initialize(options: BacktraceConfiguration): BacktraceClient { | ||
| this._instance = this.builder(options).build(); | ||
| return this._instance; | ||
| } | ||
|
|
||
| public static get instance(): BacktraceClient { | ||
| if (!this._instance) { | ||
| throw new Error('BacktraceClient is uninitialized. Call "BacktraceClient.initialize" function first.'); | ||
| } | ||
| return this._instance; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { Component, ErrorInfo, ReactElement, ReactNode, isValidElement } from 'react'; | ||
| import { BacktraceClient } from './BacktraceClient'; | ||
|
|
||
| type RenderFallback = () => ReactElement; | ||
|
|
||
| export interface Props { | ||
| children: ReactNode; | ||
| fallback?: ReactElement | RenderFallback; | ||
| } | ||
|
|
||
| export interface State { | ||
| hasError: boolean; | ||
| error?: Error; | ||
| } | ||
|
|
||
| export class ErrorBoundary extends Component<Props, State> { | ||
| private _client: BacktraceClient; | ||
| constructor(props: Props) { | ||
| super(props); | ||
| this.state = { | ||
| hasError: false, | ||
| error: undefined, | ||
| }; | ||
| // grabbing here so it will fail fast if BacktraceClient is uninitialized | ||
| this._client = BacktraceClient.instance; | ||
| } | ||
|
|
||
| static getDerivedStateFromError(error: Error) { | ||
| return { hasError: true, error }; | ||
| } | ||
perf2711 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| componentDidCatch(error: Error, info: ErrorInfo) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we also add accessors like public or private? We use it everywhere in the code. Is there any reason why we shouldn't continue doing that here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call I can add them
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in latest PR |
||
| this._client.send(error); | ||
| } | ||
|
|
||
| render() { | ||
| const { fallback, children } = this.props; | ||
|
|
||
| if (!this.state.hasError) { | ||
| return children; | ||
| } | ||
|
|
||
| const fallbackComponent = typeof fallback === 'function' ? fallback() : fallback; | ||
|
|
||
| if (fallbackComponent && isValidElement(fallbackComponent)) { | ||
| return fallbackComponent; | ||
| } | ||
|
|
||
| // no or invalid fallback | ||
| return null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,3 @@ | ||
| export * from '@backtrace/browser'; | ||
| export * from './ErrorBoundary'; | ||
| export { BacktraceClient } from './BacktraceClient'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import { ErrorBoundary } from '../src/ErrorBoundary'; | ||
| import { BacktraceClient } from '../src/BacktraceClient'; | ||
|
|
||
| describe('Error Boundary', () => { | ||
| const childrenText = 'I am the children'; | ||
| const fallbackText = 'This is a fallback'; | ||
| const errorText = 'Rendering error!'; | ||
|
|
||
| function ValidComponent() { | ||
| return <p>{childrenText}</p>; | ||
| } | ||
|
|
||
| function ErrorComponent() { | ||
| throw new Error(errorText); | ||
| return <p>{childrenText}</p>; | ||
| } | ||
|
|
||
| function Fallback() { | ||
| return <p>{fallbackText}</p>; | ||
| } | ||
|
|
||
| const fallbackFunction = () => <Fallback />; | ||
|
|
||
| describe('With BacktraeClient uninitialized', () => { | ||
| it('Should throw an error when BacktraceClient is uninitialized and an ErrorBoundary is used', () => { | ||
| expect(() => | ||
| render(<ErrorBoundary fallback={<Fallback />}>{<ErrorComponent />}</ErrorBoundary>), | ||
| ).toThrowError(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('With BacktraceClient initialized', () => { | ||
| let client: BacktraceClient; | ||
| beforeEach(() => { | ||
| client = BacktraceClient.initialize({ | ||
| url: `https://submit.backtrace.io/universe/token/json`, | ||
| name: 'test', | ||
| version: '1.0.0', | ||
| }); | ||
| }); | ||
|
|
||
| it('Should not throw an error when BacktraceClient is initialized and an ErrorBoundary is used', () => { | ||
| expect(() => | ||
| render(<ErrorBoundary fallback={Fallback}>{<ValidComponent />}</ErrorBoundary>), | ||
| ).not.toThrowError(); | ||
| }); | ||
|
|
||
| it('Should render children', () => { | ||
| render(<ErrorBoundary fallback={Fallback}>{<ValidComponent />}</ErrorBoundary>); | ||
| expect(screen.getByText(childrenText)); | ||
| }); | ||
|
|
||
| it('Should render fallback function on rendering error', () => { | ||
| render(<ErrorBoundary fallback={fallbackFunction}>{<ErrorComponent />}</ErrorBoundary>); | ||
| expect(screen.getByText(fallbackText)); | ||
| }); | ||
|
|
||
| it('Should render fallback component on rendering error', () => { | ||
| render(<ErrorBoundary fallback={<Fallback />}>{<ErrorComponent />}</ErrorBoundary>); | ||
| expect(screen.getByText(fallbackText)); | ||
| }); | ||
|
|
||
| it('Should render nothing if no fallback is passed in and rendering error', () => { | ||
| const { container } = render( | ||
| <ErrorBoundary> | ||
| <ErrorComponent /> | ||
| </ErrorBoundary>, | ||
| ); | ||
| expect(container.firstChild).toBeNull(); | ||
| }); | ||
|
|
||
| it('Should send to Backtrace on rendering error', () => { | ||
| const clientSpy = jest.spyOn(client, 'send'); | ||
| render(<ErrorBoundary fallback={<Fallback />}>{<ErrorComponent />}</ErrorBoundary>); | ||
| expect(clientSpy).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('Should not send to Backtrace when no rendering error occurs', () => { | ||
| const clientSpy = jest.spyOn(client, 'send'); | ||
| render(<ErrorBoundary fallback={<Fallback />}>{<ValidComponent />}</ErrorBoundary>); | ||
| expect(clientSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use the initialize method? Could you take a look at you're calling builder to build the class and store it in the class instance of the client? You can do the same in the constructor, which is a more intuitive way for me.
WIth this approach, you have one way to initialize everything. Right now, we have a different way to initialize other clients and we don't know which one (builder vs client) is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose we add an initialize method to all of our clients that calls the builder. I think it would provide a good user experience as the user would not be required to know implementation details about which patterns we use to create the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call it "implementation details" to use
BacktraceBuilder.build(options)? I think it's really common pattern. I'm really against static initialize method that wraps builder. Sooner than later we end up in a situation we can't do something here.I think we should discuss this as a team. I don't like the idea of having 3 ways to initialize the client. It's a short way to generate a lot of problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some way I agree with @adamcronin42 that
.initializemay be a better method. Or the one that actually initializes Backtrace in a global context..buildcould only create the instance, and not assign it as a global error handler.