feat: Allows onError callback to not use event handler #98
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
+ Coverage 96.65% 96.69% +0.03%
==========================================
Files 11 11
Lines 838 846 +8
Branches 281 284 +3
==========================================
+ Hits 810 818 +8
Misses 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ): ComponentDefinition { | ||
| const regions = props.filter(prop => prop.type === 'React.ReactNode'); | ||
| const events = props.filter(prop => prop.name.match(/^on[A-Z]/)); | ||
| const events = props.filter(prop => prop.name.match(/^on[A-Z]/) && prop.name !== 'onError'); |
There was a problem hiding this comment.
Why such fundamental change instead of simply using a different name in that API, reportError for example?
There was a problem hiding this comment.
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.
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.
The error boundary is not a typical component either, but I am happy to discuss it.
| import { describe, expect, test } from 'vitest'; | ||
| import { buildProject } from './test-helpers'; | ||
|
|
||
| describe('Error boundary', () => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
The onError callback needed for error boundaries (see [R0opA8eVEt6T]) should not use the event handler abstraction. It needs to therefore be exempted from the existing rule that requires event handler type for all callback that start from "on".
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.