-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Allows onError callback to not use event handler #98
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 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import * as React from 'react'; | ||
|
|
||
| export interface ErrorBoundaryProps { | ||
| /** | ||
| * React content. | ||
| */ | ||
| children: React.ReactNode; | ||
| /** | ||
| * Callback that fires when an error is captured. | ||
| */ | ||
| onError: (error: Error, errorInfo: React.ErrorInfo) => void; | ||
| } | ||
|
|
||
| export default function ErrorBoundary({ children }: ErrorBoundaryProps): JSX.Element { | ||
| return <div>{children}</div>; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "extends": "../tsconfig.json", | ||
| "compilerOptions": { | ||
| "rootDir": "." | ||
| }, | ||
| "include": ["./**/*.tsx"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html | ||
|
|
||
| exports[`Error boundary > should have correct properties definitions 1`] = ` | ||
| [ | ||
| { | ||
| "dashCaseName": "error-boundary", | ||
| "description": undefined, | ||
| "events": [], | ||
| "functions": [], | ||
| "name": "ErrorBoundary", | ||
| "properties": [ | ||
| { | ||
| "analyticsTag": undefined, | ||
| "defaultValue": undefined, | ||
| "deprecatedTag": undefined, | ||
| "description": "Callback that fires when an error is captured.", | ||
| "i18nTag": undefined, | ||
| "inlineType": { | ||
| "name": "(error: Error, errorInfo: React.ErrorInfo) => void", | ||
| "parameters": [ | ||
| { | ||
| "name": "error", | ||
| "type": "Error", | ||
| }, | ||
| { | ||
| "name": "errorInfo", | ||
| "type": "React.ErrorInfo", | ||
| }, | ||
| ], | ||
| "returnType": "void", | ||
| "type": "function", | ||
| }, | ||
| "name": "onError", | ||
| "optional": false, | ||
| "systemTags": undefined, | ||
| "type": "(error: Error, errorInfo: React.ErrorInfo) => void", | ||
| "visualRefreshTag": undefined, | ||
| }, | ||
| ], | ||
| "regions": [ | ||
| { | ||
| "deprecatedTag": undefined, | ||
| "description": "React content.", | ||
| "displayName": undefined, | ||
| "i18nTag": undefined, | ||
| "isDefault": true, | ||
| "name": "children", | ||
| "systemTags": undefined, | ||
| "visualRefreshTag": undefined, | ||
| }, | ||
| ], | ||
| "releaseStatus": "stable", | ||
| "systemTags": undefined, | ||
| }, | ||
| ] | ||
| `; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { describe, expect, test } from 'vitest'; | ||
| import { buildProject } from './test-helpers'; | ||
|
|
||
| describe('Error boundary', () => { | ||
|
Member
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. I'd suggest adding a test for the onError behavior. You could trigger a condition that causes onError to be called and then verify it with toHaveBeenCalledWith
Member
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. The changes here validate that the onError is added to the generated documentation (validated with the snapshot test), but the actual error boundary implementation is here: cloudscape-design/components#3736, and it does include the coverage on the onError behaviour. |
||
| test('should have correct properties definitions', () => { | ||
| const result = buildProject('error-boundary'); | ||
| expect(result).toMatchSnapshot(); | ||
| }); | ||
| }); | ||
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 such fundamental change instead of simply using a different name in that API,
reportErrorfor example?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.
The onError is used by the react-error-boundary and an internal package, so the teams are familiar with that one already.
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.
This breaks an existing convention of our components that people are familiar with as well.
Breaking the components naming convention needs a separate discussion. The PR is on hold for now.
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.
The error boundary is not a typical component either, but I am happy to discuss it.