-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[RNMobile] Support POST requests #49371
Conversation
Size Change: 0 B Total Size: 1.34 MB ℹ️ View Unchanged
|
Flaky tests detected in 58c814c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4595244702
|
/wp\/v2\/search\?.*/i, | ||
/oembed\/1\.0\/proxy\?.*/i, | ||
], | ||
POST: [ /wpcom\/v2\/(media)\/.*/i ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this past PR, I realized that we should avoid introducing WPCOM endpoints. However, this restriction affects non-core blocks like Jetpack blocks that heavily rely on WPCOM endpoints. Hence, we should refactor this part and probably define/expand the supported endpoints in Gutenberg Mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it'd be great if this could be defined in Gutenberg Mobile, I'm not sure if we can justify including the WPCOM endpoint here. @fluiddot, were you thinking of refactoring this specific PR or were you thinking of this being done separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it'd be great if this could be defined in Gutenberg Mobile, I'm not sure if we can justify including the WPCOM endpoint here. @fluiddot, were you thinking of refactoring this specific PR or were you thinking of this being done separately?
@SiobhyB I agree that it's not ideal to introduce a WPCOM endpoint here. Since the PR is related to POST requests, in order to unblock it, I'm thinking to remove this change and work on a separate PR to introduce it via GB-mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removal applied in b5d54a6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking to remove this change and work on a separate PR to introduce it via GB-mobile.
@fluiddot, that makes sense to me! I'll go ahead to approve this PR with that understanding 🙇♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SiobhyB I created the following PRs to introduce the WPCOM endpoint:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving following the discussion here. 👏 🚢
Related PRs:
WordPressOrgRestApi
wordpress-mobile/WordPressKit-iOS#589What?
Support POST requests in
apiFetch
function.Why?
This change will allow the native version of the editor to make POST requests, currently was limited to GET requests.
How?
A new function called
postRequest
has been added to the React Native bridge. Note that the POST request implementation is mainly handled in the host app.Testing Instructions
Follow the testing instructions from wordpress-mobile/gutenberg-mobile#5596.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A