Conversation
siteSlug and extraHTTPHeaders were added to be used on apiFetch
|
Even though it's a draft, please feel free to comment on this approach 👍 |
| } | ||
|
|
||
| if let extraHeaders = dataSource.extraHTTPHeaders { | ||
| initialProps["extraHTTPHeaders"] = extraHeaders |
There was a problem hiding this comment.
I would try to keep auth tokens away from initial props since react native seems to aggressively log those, and we shouldn’t log tokens
There was a problem hiding this comment.
Yes that makes sense. This was the easier way to make it work.
And also we will need to refresh it when the user switches the blog from the editor, avoiding fetching it on every network call.
src/api-fetch-setup.js
Outdated
| /** | ||
| * External dependencies | ||
| */ | ||
| import { fetchRequest } from 'react-native-gutenberg-bridge'; |
There was a problem hiding this comment.
Is this from an older attempt or I’m missing where it’s used?
There was a problem hiding this comment.
Yes, this was part of the tests making network from the parent apps. I plan to post a PR with this other approach.
src/api-fetch-setup.js
Outdated
| export default ( setupApiFetch = ( siteSlug = '', headers = {} ) => { | ||
| apiFetch.use( apiFetch.createRootURLMiddleware( 'https://public-api.wordpress.com/' ) ); | ||
| apiFetch.use( ( options, next ) => wpcomPathMappingMiddleware( options, next, siteSlug ) ); | ||
| apiFetch.setFetchHandler( ( options ) => fetchHandler( options, headers ) ); |
There was a problem hiding this comment.
You don't need a custom fetch handler to inject headers, I got this one working in our meetup:
WordPress/gutenberg@109182d
There was a problem hiding this comment.
Sounds good!
Implemented a simpler approach, since we are passing a generic "extra headers" prop, not necessarily containing auth related fields.
|
|
||
| export default ( setupApiFetch = ( siteSlug = '', headers = {} ) => { | ||
| apiFetch.use( apiFetch.createRootURLMiddleware( 'https://public-api.wordpress.com/' ) ); | ||
| apiFetch.use( ( options, next ) => wpcomPathMappingMiddleware( options, next, siteSlug ) ); |
There was a problem hiding this comment.
Even if we don't have .org auth ready, we should make these conditional. The app should provide the root URL, and depending on whether that's *.wordpress.com or a site URL, the path mapping (and public-api root url) should be added automatically.
This also means that siteSlug might be inferred from the passed root URL, as it's only needed for WordPress.com requests.
|
Taking this out of the line comment because it's important enough:
I'd rather deal with auth separately and explicitly. I know this might seem premature but I've been scarred before. If you look at the documentation for api-fetch, it supports passing an absolute Core doesn't have to worry about that, because it uses cookie-based authentication, and browsers will do the worrying about where to send each cookie. My middleware implementation was also too naive, and didn't check where it was sending the request before injecting a token 🤦♂ I think we have two options:
I think we can easily build the first today, but I'd love for us to invest in the second for a longer term solution. |
That makes total sense. Will take a look at it. This is one of the reasons I also like the option of doing the networking bits from the parent apps: #1455 But I'm not in favor of any option in particular (yet), so I'm happy to continue improving this approach too. |
|
Closing this in favor of #1455 |
This PR implements apiFetch to get the media object, and set the corresponding image URL when the image size is changed on an image block.
The wpcom specific code is located in this repo. And the needed information (authentication headers and site id/slug) are provided via initial props from the parent app.
This can only be tested on WPiOS for now, and only works on WPCom sites, as a first step.
gutenberg PR: WordPress/gutenberg#17979
WPiOS PR: wordpress-mobile/WordPress-iOS#12706
To test:
Update release notes:
RELEASE-NOTES.txt.