Pass Android Fetch Responses Across the Bridge as Objects

[mchowning]

Android fetch responses are passed from native to JS as a String. iOS passes an object instead. I think I may have used a String on Android because I was not aware that React Native had helper methods for creating WritableMaps and WritableArrays like we do for errors. Let's give this a second look to see if we can switch Android to pass an object like iOS.

[SergioEstevao]

@mchowning it will be nice to fix this to improve our network support in GB-mobile.

[mchowning]

@mchowning it will be nice to fix this to improve our network support in GB-mobile.

I'll take a look at this when I'm doing the auth work.

[mchowning]

I'll take a look at this when I'm doing the auth work.

Well that didn't happen. 😞 Removing my assignment to this, but I'll keep it in mind and try to pick it back up soon.

[SiobhyB]

👋 @mchowning, I've been looking into this and wondered whether you might be able to help with some questions that I have.

---

Initial discovery and context for my questions:

I found that this issue was originally spun up following a discussion around the implementation of fetchRequest in the Latest Posts block to fetch categories. At a later stage, in https://github.com/WordPress/gutenberg/pull/21025, fetchRequest was replaced with Gutenberg's apiFetch, this was the result of this discussion in which the following was said about fetchRequest:

This shouldn't be used directly and instead, it should use the GB apiFetch interface. This will provide support for retries and cache of the data.

Using apiFetch negates the need for casting the Android response, as that's handled in the api-fetch-setup.js file. It may still be useful to fix this at the root, however.

---

Questions:

Given the above, my first question is whether this would still be a useful fix? I'm assuming it would be if, in the future, fetchRequestis called from anywhere else in the codebase, but am not clear if there is a possible use case for that.

If this is indeed still a worthwhile fix, I have a (very rough) draft PR at https://github.com/WordPress/gutenberg/pull/34391 where I've made an (equally rough) list of "things I've tried" so far while trying to figure out a fix. I'm leaning towards exploring the third item from my list further ("Passing the response as a JsonObject"). Do you think that seems like a good idea? I'd be really grateful for any thoughts or ideas you may have. 🙇‍♀️

[mchowning]

Thanks for looking into this and giving a great description of what you've tried in your draft PR @SiobhyB !

Using apiFetch negates the need for casting the Android response, as that's handled in the api-fetch-setup.js file.

You're absolutely right. The main point behind this ticket was to try to avoid there needing to be a type check every time we used fetchRequest. As you found, that problem has been solved! 🎉

It may still be useful to fix this at the root, however... Given the above, my first question is whether this would still be a useful fix? I'm assuming it would be if, in the future, fetchRequestis called from anywhere else in the codebase, but am not clear if there is a possible use case for that.

It would still be nice to fix this to make the platforms consistent, but it is no longer really what I would consider "technical debt" since we're only doing the conversion in one place (inside api-fetch). If we can't make the change to have Android also pass back an object quickly/easily, it's probably not worth it.

If this is indeed still a worthwhile fix, I have a (very rough) draft PR at WordPress/gutenberg#34391 where I've made an (equally rough) list of "things I've tried" so far while trying to figure out a fix. I'm leaning towards exploring the third item from my list further ("Passing the response as a JsonObject"). Do you think that seems like a good idea?

The response comes back from FluxC to WPAndroid as a nullable JsonElement, so I don't think we'll need to do any parsing--instead, we can just pass that JsonElement back through Android until we get to the point where it needs to cross the bridge because iirc we can't pass a JsonElement (or its subclass JsonObject) back to RN.

There doesn't seem to be any out-of-the box solution for converting a JsonObject to something that RN can pass across the bridge that I can find. The best bet would probably be to convert the JsonObject to a Bundle because that is a problem that people have had to solve outside the context of React Native (random untested, but promising looking, result that I found googling). As that example shows though, this is not a super-simple thing to do since we won't know the structure of the son object so we would need to ensure that our conversion would work for any "shape" of json.

Once we get a Bundle we can convert that to a WritableMap using the same Arguments.makeNativeMap method we use to send an error.

My suggestion would be to give this a pretty tight timebox--if it's not something we have "fixed" in a few hours, it's probably not worth it, and we can just close this issue imo. Don't hesitate to let me know if you have any questions or if anything I said was confusing. 🙂

[SiobhyB]

Thanks so much for this Matt! 🙇‍♀️ It sounds like I was on the right path with my initial explorations, which is reassuring, but I haven't been able to get this to work. I've gone over the suggested timebox of a few hours by quite a big margin, as this was my main focus of last week. I'd really love to get to the bottom of what I'm missing, given I've invested quite a lot of time into this already. I have a pair programming session set up with @fluiddot for Thursday to see if we can figure it out together. If we're not able to figure out a way towards a solution in that session, I'll go ahead to close this issue.

[mchowning]

I'd really love to get to the bottom of what I'm missing

👍 Definitely feel free to devote some more time than my suggested timebox on this if you're finding it interesting and a good opportunity for learning.

[SiobhyB]

As an update: Talked through some more options to explore with @fluiddot in our pair programming session, with converting the JSON to a map before passing it over the bridge seeming like the most promising way forward. I wasn't able to get this to work after spending a bit more time on it, however. I'd like to see this through as I feel pretty invested, but I also have other open PRs and tasks to prioritize. I'll therefore remove my assignment, but have this on my "paused" list of PRs to revisit if I have downtime and if someone else doesn't get to this before me.