[RNMobile] Prevent unhandled promise rejection warning related to canUser resolver #39144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
[Type] Bug
|
Size Change: -195 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
This patch certainly avoids the crash, but do we know why there is no |
Oh, sorry for not providing the entire context. In the native version, all the network logic is managed by the WordPress app. For succeeded network requests, we only get the result object of the response hence, we don't receive any properties of the HTTP request like the headers. However, we do receive it on failed requests, mostly in order to determine the error type via the HTTP status code. As an example, let me share some references to the iOS native code to expand the explanation:
I'd agree that it would be quite helpful to also receive the HTTP response object, as it would prevent issues like the one addressed by this PR. But I'm afraid that until we refactor the network logic, in the native version we have this limitation. In fact, I also bumped into this problem on other issues like wordpress-mobile/gutenberg-mobile#2661, related to the unbounded queries functionality from fetch-all-middleware, which is not supported due to this same problem.
Right, the |
That happens in the web version, too, we get only the parsed response JSON body and nothing else. However, when the apiFetch( { path, method: 'OPTIONS', parse: false } )This means that we want to get the raw
Yeah if the mobile app is content with |
jsnajdr
approved these changes
Mar 2, 2022
| @@ -292,7 +292,7 @@ export const canUser = ( action, resource, id ) => async ( { dispatch } ) => { | |||
| return; | |||
| } | |||
|
|
|||
| const allowHeader = response.headers.get( 'allow' ); | |||
| const allowHeader = response.headers?.get( 'allow' ); | |||
There was a problem hiding this comment.
Let's add a comment explaining that the ?. operator needs to be used because the React Native apiFetch doesn't return the expected result.
There was a problem hiding this comment.
I added a comment related to this in b268726.
That's interesting, it would be great if we could match the same behavior in the native version, and totally agree that the network logic should respect that 👍 .
Yep, I know it's not ideal to have
I totally agree, I'll open a ticket as a follow-up of this and highlight the importance of providing the same behavior, in order to prevent future issues. Thanks @jsnajdr for taking a look at the PR 🙇, I'll add the comment you mentioned in #39144 (comment), and then we could merge the fix. |
In relation to this comment, I'd like to share that I opened #39182. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As outlined in #39143 (comment), the issue has been addressed by using the optional chaining operator when retrieving the headers from the response.
Testing Instructions
Screenshots
N/A
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).