🐛 [bento][amp-social-share] Preact component should not throw error #30200
Conversation
| @@ -54,31 +54,34 @@ export function SocialShare({ | |||
| checkedWidth, | |||
| checkedHeight, | |||
| checkedTarget, | |||
| errorState, | |||
There was a problem hiding this comment.
Per discussion. I don't think we need to track errorState. We need to simplify it to a point of almost no error processing done in a component. The most important consequence is that we likely need to drop tests with expect(() => mount(jsx)).to.throw.
There was a problem hiding this comment.
Thanks @dvoytenko . Updated revision is up. I still included some minimal error checking which results in the component immediately returning null and does not render anything.
The corresponding tests check for an empty render instead of an error being thrown.
I have included a console.warn to notify the publisher and check for this in the tests. These can be removed if you think it makes more sense. Thanks!!
krdwan
force-pushed
the
social-share-errors
branch
from
f5c3b88
to
48347fe
Compare
|
|
||
| it('warns when the required "type" attribute is not provided', () => { | ||
| const consoleOutput = []; | ||
| const mockedWarn = (output) => consoleOutput.push(output); |
| if (type === undefined) { | ||
| throw new Error(`The type attribute is required. ${NAME}`); | ||
| displayWarning(`The type attribute is required. ${NAME}`); |
There was a problem hiding this comment.
Not needed. Let's update type.
| `An endpoint is required if not using a pre-configured type. ${NAME}` | ||
| ); | ||
| return { |
There was a problem hiding this comment.
Can we return null instead?
| @@ -120,27 +134,23 @@ function processChildren(type, children, color, background, size) { | |||
| * @param {number|string|undefined} width | |||
| * @param {number|string|undefined} height | |||
| * @param {JsonObject|Object|undefined} params | |||
| * @return {{ | |||
| * @return {null|{ | |||
| @@ -18,7 +18,7 @@ | |||
|
|
|||
| /** | |||
| * @typedef {{ | |||
| * type: (string|undefined), | |||
| * type: (!string), | |||
There was a problem hiding this comment.
@dvoytenko I updated this to non-nullable string. Is this what you meant by strict string?
There was a problem hiding this comment.
Strict string is just string. I.e. no ? or |null or |undefined or any other |. The ! is not needed for primitive types, and I think the Closure styleguide complains about it.
There was a problem hiding this comment.
Got it, thanks for clarifying!
…mpproject#30200) * Preact component updated to not throw error * Cleaner early exit on error for Social Share * Remove errorState from social share * checkProps should return null * Update preact types file to non-nullable string for type prop * Update syntax for nullable object return type * Remove test for checking type prop is provided * Update strict string without ! in social-share.type
Tracking Issue: #28283
Updated Preact component and automated tests for Social Share.
Preact component should not throw an error.