React native/pass http error code#11001
Conversation
|
You can test the changes on this Pull Request by downloading the APK here. |
| putString("message", it) | ||
| } | ||
| } | ||
| onError(bundle) |
There was a problem hiding this comment.
I don't know if it's possible but what about sending a Pair instead? It seems less error prone than a bundle 🤷♂
There was a problem hiding this comment.
Pair is actually what I used for my first implementation of this. 😀
I decided to use a Bundle instead because I had to convert the Pair to a Bundle-like React Native object (WritableMap) that can be sent across the javascript bridge to React Native (like with saving instance state for Activities you can only send a few special kinds of objects across the javascript bridge). That conversion involves exactly the same kind of putString(...) and putInt(...) calls that I am doing here, so it didn't seem like Pair was gaining me anything, it was just moving the place where I did the "risky" putX(...) calls. Using a Bundle directly seemed to make since on account of the fact that React Native has a helper method for converting a Bundle to a WritableMap automatically.
There is a helper for converting Map<String, Object to WritableMap as well, but that actually seemed less safe since we would have no type safety to insure that we only put objects into the map that could be sent across the bridge.
Using Bundle directly also seemed like a better longterm approach since if/when (most likely when) we end up wanting to pass a third piece of information from the error, I think we would need to convert this back from a Pair to a Bundle or start nesting Pair<Pair<>>. 😱 (
An alternative approach would be to change WordPress-Editor to use react-native-gutenberg-bridge as an api dependency instead of an implementation dependency. That would allow us to define a well-typed object for the relevant error information in react-native-gutenberg-bridge and use that object here in WordPress-Android (transitively, via the Editor module). I feel like this might actually end up being the best longterm solution, but it adds React Native dependencies to the WordPress app itself (as opposed to just being implementation dependencies of the Editor module) and I don't feel comfortable that I've thought through all the implications of doing that yet. wdyt?
marecar3
left a comment
There was a problem hiding this comment.
LGTM!
Probably we want to update this with the latest develop?
6ad955f to
3ca029e
Compare
Updates to version of FluxC that returns full error object for failed requests. Takes that full response and extracts the http error code from it and passes that along to mobile gutenberg.
Why We're Using a
BundleTo Pass Error InformationThis error information has to be passed from the
WordPressmodule to theWordPressEditormodule and then to thereact-native-gutenberg-bridgemodule. In other words,WordPressdepends onWordPressEditor, which depends onreact-native-gutenberg-bridge.WordpressEditor, however, only hasreact-native-gutenberg-bridgeas an "implementation" dependency (I'm guessing this was done for the very good reason of wanting to keep the core app clean of any React Native dependencies). As a result,WordPressdoes not know anything aboutreact-native-gutenberg-bridgeandreact-native-gutenberg-bridgeknows nothing about eitherWordPressorWordPressEditor.This means that if we made a class to hold the error data, no matter where we put it, one of either
WordPressorreact-native-gutenberg-bridgewould not be able to use it. Instead, we would be forced to have one class inWordPressEditorfor passing the error data fromWordPresstoWordPressEditor. Then, insideWordPressEditor, we would have to convert that first data class to a new data class fromreact-native-gutenberg-bridge, so that the information could be passed to that module (becausereact-native-gutenberg-bridgewould not know about the class fromWordPressEditor).Not only would we need these two, essentially identical, data classes, however. We would also need to convert the data class from
react-native-gutenberg-bridgeinto a kind of object that can be sent over the javascript bridge to React Native (WritableMap). Fortunately, React Native has a helper method to automatically convert aBundleinto aWritiableMap.FWIW, one other option would be to use a
Map<String, Object>instead of aBundle. React Native also has a helper to convert that kind of object to aWritableMap. I did not go with that approach because that has even less type-safety than aBundle.If you think there's a better way to handle this, please let me know. It just seemed that using a well-typed data class with these errors would require a substantial amount of boilerplate for little, if any, gain since that data would then just have to be convered into a
WritableMap, which is essentially the same thing as aBundle.To Test
See test steps in wordpress-mobile/gutenberg-mobile#1686
PR submission checklist
I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txtif necessary.