-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Improve castError handling of non strings #13315
Improve castError handling of non strings #13315
Conversation
Big +1 to a solution like this. |
I'm struggling to appreciate from its original introduction with #8096 why it is we cast to error at all, particularly as we start to encounter issues surrounding the fact that a thrown value can take any form. |
* @return {Error} An instance of ReduxRoutineResponseError | ||
* @constructor | ||
*/ | ||
export function ReduxRoutineResponseError( errorResponse, ...args ) { |
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.
Is there any reason this could not be class extends Error
?
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.
Babel doesn't handle extending built-in classes (https://babeljs.io/docs/en/caveats#classes)
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.
Babel doesn't handle extending built-in classes.
In what sense?
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 can only speak from experience when I tried to extend built-ins in code I've written. I've gotten compile errors or I've seen errors when throwing the exception. Switched to doing ES5 and no issues.
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.
When I saw the babel docs, I figured that meant they don't fully support extending built-ins.
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 didn't see the caveats link edit 'til after. It does sound like it'd be an issue, though the generated code when actually trying to extend Error
looks much like what you've hand-recreated here.
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'm not too familiar with the online babel generator, is it using the full babel browser compiler or does it simulate transpiling via a build tool? I wonder if that might be a factor here (i.e. the online tool you linked to considers the browser in use as well).
It's also possible that the issues I experienced have been resolved in the more recent version of Babel (and they just haven't updated their caveats yet). Regardless, I'm not certain what harm there is in using ES5 in this case?
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.
Personally, I'd much prefer to just extend the class, but I'm wary based on my experience.
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.
Should note, your link is also referencing Babel version 6.25. v7.2 has more boilerplate it looks like.
I'm guessing that the intention was so that generators could just add a try/catch block and handle thrown errors as opposed to dealing with potentially mixed response. So the idea is to give consuming code something predictable in the face of error-like behaviour. |
0c6b094
to
1af0005
Compare
I can see both sides of the argument, but in reflection, it seems like the most useful thing would be to bubble up the actual thrown value. Or, at least, it doesn't seem like we're gaining much by keeping the current behavior, and as evidenced by the changes here, it's forcing us to jump through hoops to achieve with varying value shapes. The main caveat would be whether it would be considered a breaking change. We never documented the current behavior, and any thrown errors would continue to be received as an error. There's a middle-ground option too where we only coerce to |
I'm not sure how useful this is. How would consuming code handle this then? A try/catch won't work right (because there's no certainty an Error will be thrown)? |
I'm not sure I follow. There's nothing about Try, for example, in the console: try {
throw 12;
} catch ( error ) {
console.log( 'Thrown: ' + error );
} |
Oooh ya, sorry. So, then the issue will just be for consuming code to determine whether its an instance of |
The last of these is most appealing to me both in minimizing the surface area we're responsible for maintaining, and in predictability of what's received by the |
Ya agreed. It seems you were the author of the original code, are you fine with me dropping I'm not sure this should be considered breaking. It's unlikely anyone is checking to see if the returned value is an Error but there is the potential |
The one downside we do have to this approach though is the predictability of always having |
Yes.
Maybe, which is my remaining point of hesitation. Trying to consider the scenario of breakage requires some very contrived examples, and are complemented by just as many "buggy" examples of the current behavior.
It was never documented to surface itself as an |
I'm still trying to process #12375 (comment) and whether there's a complement here in changing how we handle "error" responses from Related: |
Yea I agree. I don't think we should abandon work on improving apiFetch response error surfacing and it be complementary to what's done here. However, I still think that regardless of what happens in apiFetch we want to make sure the actual response makes its way to consuming code in the generator implementing a fetch control (and catching the thrown error from runtime). |
- introduce ReduxRoutineResponseError, a custom error handler for non string error values. The “response” is added to a `response` property on this new error handler thus exposing arbitrary responses to any client code wanting to extract from. - implement ReduxRoutineResponseError in `castError` so that this error object is always returned when the error value is not already an instance of Error.
bfaf761
to
803f848
Compare
@aduth, the latest push (as per our convo in here):
I'm taking the approach that this is not a breaking change. Although I do see that it's still debateable on whether this is or not, the reality is that the worse that would happen is consuming code calls @TimothyBJacobs do you have any thoughts on whether this should be classified as breaking or not? |
I agree with your reasoning @nerrad. |
packages/redux-routine/CHANGELOG.md
Outdated
@@ -2,7 +2,9 @@ | |||
|
|||
### Bug Fixes | |||
|
|||
- Fix unhandled promise rejection error caused by returning null from registered generator ([#13314](https://github.com/WordPress/gutenberg/pull/13314) | |||
- Fix unhandled promise rejection error caused by returning null from registered generator ([#13314](https://github.com/WordPress/gutenberg/pull/13314)) | |||
- Removed `castError`. The middleware will now simply throw the value exposed on a Promise reject or thrown error rather than coercing to an instance of `Error`. Consuming code can now access whatever value is thrown directly. |
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.
castError
was never exposed to a consumer, so they needn't care about it, at least by name.
- Removed `castError`. The middleware will now simply throw the value exposed on a Promise reject or thrown error rather than coercing to an instance of `Error`. Consuming code can now access whatever value is thrown directly. | |
- The middleware will no longer attempt to coerce an error to an instance of `Error`, and instead passes through the thrown value directly. |
Outside of the above small revisions, I think this is ready to land. |
Co-Authored-By: nerrad <darren@roughsmootheng.in>
changes made, just need an approval before a merge. |
…rnmobile/372-add-title-to-gutenberg-mobile * 'master' of https://github.com/WordPress/gutenberg: (56 commits) Save package-lock.json file changes (#13481) Plugin: Deprecate gutenberg_add_responsive_body_class (#13461) Add speak messages to the feature toggle component. (#13385) Plugin: Deprecate gutenberg_kses_allowedtags (#13460) Plugin: Deprecate gutenberg_bulk_post_updated_messages (#13472) Plugin: Avoid calling deprecated gutenberg_silence_rest_errors (#13446) Plugin: Deprecate gutenberg_remove_wpcom_markdown_support (#13473) Fix: Categories block: add custom classes only to wrapper (#13439) is-shallow-equal: Use ES5 ruleset from eslint-plugin module (#13428) Update and Organize Contributors Guide per #12916 (#13352) Dismissible-notices: fix text overlapping icon (X) (#13371) Framework: Remove 5.0-merged REST API integrations (#13408) Plugin: Remove 5.0-merged block registration functions, integrations (#13412) Framework: Bump minimum required WP to 5.x (#13370) [Mobile] Improve keyboard hide button (#13415) Improve castError handling of non strings (#13315) Fix: File block add custom class (#13432) Consider making Fullscreen Mode effects visible only on larger screens (#13425) Update plugin version to 4.9.0 (#13436) DateTimePicker: fix prop warning for (#12933) ...
This pull removes castError from reduxRoutine and just passes along the value thrown instead of potentially coercing to an error. - Exposes values to consuming code. - Less opinionated about what happens with the error.
This pull removes castError from reduxRoutine and just passes along the value thrown instead of potentially coercing to an error. - Exposes values to consuming code. - Less opinionated about what happens with the error.
Description
Currently there is a flaw with
redux-routine/cast-error
which converts the incomingerror
argument to anError
object if it already is not. The flaw is thatError
expects themessage
parameter to be a string, so any plain objects will be coerced to[Object object]
(which then gets returned by themessage
property).This has been reported already in #12375 but the suggested proposed fix there (#12376) still does not resolve this inherent flaw in
castError
.In this pull:
ReduxRoutineResponseError
is used instead ofError
. This will consistently expose any incoming error value on theresponse
property so consuming code can utilize it.Advantage of this approach is it exposes to client code whatever was returned by a promise as an error. So its not specific to
apiFetch
error responses.Example implementation by client code:
How has this been tested?
Types of changes
ReduxRoutineResponseError
is still an instance of Error.Checklist: