Pass full error for use by Gutenberg #1459
Conversation
| } | ||
| } | ||
| } | ||
| class Error(val error: BaseNetworkError) : ReactNativeFetchResponse() |
There was a problem hiding this comment.
Actually, after looking into the WPAndroid part, since we're only using the error status code and the message, could we keep the logic above here and pass these two fields here? I'd rather not use this object in WPAndroid (if possible).
There was a problem hiding this comment.
That's certainly possible. Admittedly I was trying to do a bit of future-proofing with this change.
I wanted to avoid having to do FluxC changes every time we need to send more or different error information to React Native. Since this api will be used for a number of different calls by gutenberg (only one of which we've actually looked at so far), I think it's pretty safe to say that we'll probably end up needing to make some changes to the error information than we are now. The required error information is also heavily dependent on the web side of gutenberg, so it's pretty safe to say that in the future there will be changes on the web side that will require us to make changes to FluxC if it is only passing the currently-relevant parts of the error.
The fact that each time we want to change the error information we're sending back requires a change to FluxC felt a bit like a leak of the implementation details of gutenberg into FluxC. Instead, since for successful calls we're passing the raw response back to RN in the form of json (instead of trying to parse it and only passing back the specific things that I need), my thinking was that it made sense to take a similar approach with the error by just passing all the error information back from FluxC. Admittedly, to be perfectly consistent I would have converted the error to json like we do for the response and pass that back, but I didn't want to do that for obvious reasons.
Just to give a bit of flavor for where I'm coming from: the change to add the http error code to the error response we're sending back to React Native required changing around 10 lines in a single file on WiOS. Doing the same thing on Android required changing 14 files, coordinated across three different repositories. Admittedly, most of the Java changes are just boilerplate, but having to coordinate them between three repositories is still a fair bit of overhead for a relatively small change.
My hope with these PRs was to drastically reduce the number of changes needed whenever we wanted to change the error information sent back to react native. In particular, I believe the way that I have updated the handling in FluxC and WPAndroid in these PRs, it would only require a change to a single file in WPAndroid (ReactNativeRequestHandler) the next time we need to change the error response that is passed back to React Native, instead of the 14 files that needed to be changed this time.
With all that said (sorry this has run so long), you certainly have a much better feel for FluxC than I do, so if you still think we're better off changing this to only pass back the error message and http code from FluxC, just let me know and I'll gladly make the change. 😀
There was a problem hiding this comment.
👋 @planarvoid ! Let me know if you'd like me to go ahead and make the change or if you're ok with my approach!
|
@mchowning @marecar3 Just a quick heads up that when PRs have breaking changes for WPAndroid (or WCAndroid) we try to prepare the relevant PR on that platform before merging said PR. It looks like you have already done that, but didn't quite schedule them to be merged one after another. It's not a big deal in this case because the breakage seems simple enough to ignore. (I can simply pass in a random string here to bypass the issue temporarily). However, it can sometimes be complicated and would prevent anyone else from pulling in new changes from FluxC until that PR is merged. Hope that makes sense! |
|
Thanks @oguzkocer ! It completely slipped my mind that this would be a breaking change. Sorry about that! |
Previously, we were only passing a human-readable error string to Gutenberg in the case of errors. Gutenberg now needs access to additional error information though, so this PR updates the React Native request handling to return the full error object. That way any parsing of the error that needs to be done for Gutenberg can be kept out of FluxC and instead handled closer to Gutenberg.