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

Merged
merged 10 commits into from Jan 23, 2019

Conversation

Projects
None yet
4 participants
@nerrad
Copy link
Contributor

nerrad commented Jan 14, 2019

Description

Currently there is a flaw with redux-routine/cast-error which converts the incoming error argument to an Error object if it already is not. The flaw is that Error expects the message parameter to be a string, so any plain objects will be coerced to [Object object] (which then gets returned by the message 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:

  • A custom Error object, ReduxRoutineResponseError is used instead of Error. This will consistently expose any incoming error value on the response property so consuming code can utilize it.
  • Tests were added to verify it handles array, objects etc.

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:

// a resolver catching apiFetch errors.
function* getSomething( someValue ) {
    let response;
    try {
		response = yield fetch( { path: '/some-path/?value=' + someValue } );
	} catch ( e ) {
		message = e.response.message || e.message;
    }
}

How has this been tested?

  • Unit tests, and verification in a plugin I'm working on.

Types of changes

  • non-breaking change because ReduxRoutineResponseError is still an instance of Error.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@TimothyBJacobs

This comment has been minimized.

Copy link
Contributor

TimothyBJacobs commented Jan 14, 2019

Big +1 to a solution like this.

@nerrad nerrad referenced this pull request Jan 14, 2019

Closed

Add CRUD functionality to `eventespresso/core` wp.data store. #780

9 of 10 tasks complete
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 15, 2019

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 ) {

This comment has been minimized.

@aduth

aduth Jan 15, 2019

Member

Is there any reason this could not be class extends Error ?

This comment has been minimized.

@nerrad

nerrad Jan 15, 2019

Author Contributor

Babel doesn't handle extending built-in classes (https://babeljs.io/docs/en/caveats#classes)

This comment has been minimized.

@nerrad

nerrad Jan 15, 2019

Author Contributor

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.

This comment has been minimized.

@nerrad

nerrad Jan 15, 2019

Author Contributor

When I saw the babel docs, I figured that meant they don't fully support extending built-ins.

This comment has been minimized.

@aduth

aduth Jan 18, 2019

Member

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.

This comment has been minimized.

@nerrad

nerrad Jan 18, 2019

Author Contributor

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?

This comment has been minimized.

@nerrad

nerrad Jan 18, 2019

Author Contributor

Personally, I'd much prefer to just extend the class, but I'm wary based on my experience.

This comment has been minimized.

@nerrad

nerrad Jan 18, 2019

Author Contributor

Should note, your link is also referencing Babel version 6.25. v7.2 has more boilerplate it looks like.

@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 15, 2019

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.

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.

@nerrad nerrad force-pushed the feature/improve-cast-error-handling-of-non-strings branch from 0c6b094 to 1af0005 Jan 16, 2019

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 18, 2019

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.

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 Error when the received argument is of type string.

@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 18, 2019

There's a middle-ground option too where we only coerce to Error when the received argument is of type string.

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)?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 18, 2019

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 / catch which requires that the thrown value is of type Error. This is also why I'm wondering why we don't pass through the original value.

Try, for example, in the console:

try {
    throw 12;
} catch ( error ) {
    console.log( 'Thrown: ' + error );
}
@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 18, 2019

Oooh ya, sorry. So, then the issue will just be for consuming code to determine whether its an instance of Error or not and handle accordingly. Ya that might work better then. Only coerce to Error if its a string. Or do we need even need to bother coercing then? Just throw the value?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 18, 2019

Only coerce to Error if its a string. Or do we need even need to bother coercing then? Just throw the value?

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 catch of a consumer (rather than trying to explain the behavior of "if it's a string, then it's wrapped by Error").

@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 18, 2019

Ya agreed. It seems you were the author of the original code, are you fine with me dropping castError then as well from the module? Would you consider this a breaking change (so should be recorded as such in the CHANGELOG.md)?

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 error.message is being used directly. At that point, there's already an error though...

@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 18, 2019

The one downside we do have to this approach though is the predictability of always having error.message.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 18, 2019

are you fine with me dropping castError then as well from the module?

Yes.

I'm not sure this should be considered breaking.

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.

The one downside we do have to this approach though is the predictability of always having error.message.

It was never documented to surface itself as an Error, despite the implementation awkwardly attempting to do so. To me, the more expected behavior would be to receive the actual thrown value.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 18, 2019

I'm still trying to process #12375 (comment) and whether there's a complement here in changing how we handle "error" responses from apiFetch.

Related:

@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 18, 2019

I'm still trying to process #12375 (comment) and whether there's a complement here in changing how we handle "error" responses from apiFetch.

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).

nerrad added some commits Jan 14, 2019

improve handling of non strings for errors
- 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.

@nerrad nerrad force-pushed the feature/improve-cast-error-handling-of-non-strings branch from bfaf761 to 803f848 Jan 19, 2019

@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 19, 2019

@aduth, the latest push (as per our convo in here):

  • removes castError and the middleware routine now exposes whatever error was rejected or thrown in the Promise directly.
  • updated tests to correct expectation.
  • update changelog

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 error.message which would be undefined. This happens in error conditions anyways so this should improve things for consuming code once the change is noticed.

@TimothyBJacobs do you have any thoughts on whether this should be classified as breaking or not?

@nerrad nerrad requested a review from aduth Jan 19, 2019

@TimothyBJacobs

This comment has been minimized.

Copy link
Contributor

TimothyBJacobs commented Jan 21, 2019

I agree with your reasoning @nerrad.

@@ -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.

This comment has been minimized.

@aduth

aduth Jan 22, 2019

Member

castError was never exposed to a consumer, so they needn't care about it, at least by name.

Suggested change Beta
- 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.
Show resolved Hide resolved packages/redux-routine/src/runtime.js Outdated
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 22, 2019

Outside of the above small revisions, I think this is ready to land.

aduth and others added some commits Jan 22, 2019

Update packages/redux-routine/CHANGELOG.md
Co-Authored-By: nerrad <darren@roughsmootheng.in>
@nerrad

This comment has been minimized.

Copy link
Contributor Author

nerrad commented Jan 22, 2019

changes made, just need an approval before a merge.

@nerrad nerrad requested a review from aduth Jan 22, 2019

@aduth

aduth approved these changes Jan 23, 2019

@nerrad nerrad merged commit c501353 into master Jan 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nerrad nerrad deleted the feature/improve-cast-error-handling-of-non-strings branch Jan 23, 2019

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Jan 23, 2019

daniloercoli added a commit that referenced this pull request Jan 24, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment