Packages: Replace the apiRequest module with api-fetch module #7566

youknowriad · 2018-06-27T10:36:31Z

This PR follows the core-js meeting and replaces the wp.apiRequest with a new wp.fetch module not relying on jQuery APIs but using window.fetch

Differences:

Returning regular promises
If you want to receive the entire response you need to pass parse: false
To avoid data processing, you can pass body directly

Remaining tasks

Deprecate wp.apiRequest (I'm leaving it for later, we need to settle on the deprecation strategy first)

closes #7488 #7455 #7515

joyously · 2018-06-28T15:31:27Z

packages/fetch/src/index.js

+	middlewares.push( middleware );
+}
+
+function request( options ) {


Why is it called request? Does this map to fetch somewhere?

It's just an internal name to avoid confusion with window.fetch (which can also be accessed by fetch since it's a global) used on the same file.

Maybe it should have been called apifetch then. Or wpfetch.

noisysocks

Really supportive of this change—moving away from jQuery is 💯.

There's a few places where we're treating response.json() as a promise which will cause errors. For example, the latest posts block doesn't load when I check out this branch. We're also referencing responseJSON in quite a few error handlers which isn't right. (I haven't tested this, though.)

As a developer that's familiar with fetch, I find it a little confusing that @wordpress/fetch doesn't behave exactly the same as window.fetch unless you pass it parse: false. Maybe renaming it to apiFetch would help with this? Alternatively, we could move the "magic" that we do (JSON parsing and response status error handling) into a function that works on regular Response objects.

noisysocks · 2018-06-29T00:54:11Z

components/higher-order/with-api-data/request.js

 			return {
-				body,
-				headers: getResponseHeaders( xhr ),
+				body: response.json(),


response.json() returns a promise. We'll need to wait for it to resolve here.

https://developer.mozilla.org/en-US/docs/Web/API/Body/json

noisysocks · 2018-06-29T00:59:19Z

components/server-side-render/index.js

+			.catch( ( response ) => {
+				const failResponse = {
+					error: true,
+					errorMsg: response.json().message || __( 'Unknown error' ),


response.json() returns a promise which means message here will be undefined.

https://developer.mozilla.org/en-US/docs/Web/API/Body/json

noisysocks · 2018-06-29T01:16:28Z

editor/store/effects.js

@@ -302,7 +302,7 @@ export default {
 		const { postId } = action;
 		const basePath = wp.api.getPostTypeRoute( getCurrentPostType( getState() ) );
 		dispatch( removeNotice( TRASH_POST_NOTICE_ID ) );
-		apiRequest( { path: `/wp/v2/${ basePath }/${ postId }`, method: 'DELETE' } ).then(
+		fetch( { path: `/wp/v2/${ basePath }/${ postId }`, method: 'DELETE' } ).then(


In the error handler for this request (line 317) we reference err.responseJSON which will now be undefined. We do this in most places that used apiRequest.

youknowriad · 2018-06-29T08:07:17Z

As a developer that's familiar with fetch, I find it a little confusing that @wordpress/fetch doesn't behave exactly the same as window.fetch unless you pass it parse: false. Maybe renaming it to apiFetch would help with this? Alternatively, we could move the "magic" that we do (JSON parsing and response status error handling) into a function that works on regular Response objects.

I guess I like the auto parsing to avoid repeating the same thing over and over again in all of our implementations. I think parsing is the default in 99% of use-cases, isn't it?

tofumatt · 2018-06-29T08:19:48Z

If it won't behave like fetch I think putting fetch in the name at all might be best avoided, but apiFetch is fine. Re-using apiRequest is better I think.

I definitely think we should be parsing by default; it's totally the standard use-case and it's handy to not have to worry about it! 👍

youknowriad · 2018-06-29T08:57:58Z

We can't reuse apiRequest for backwards compatibility. We can do apiFetch but I personally prefer just fetch and even without parsing the API is different. window.fetch expects two arguments while wp.fetch expects the path as part of the options, do you think we should change this?

Edit Actually, we can't change this because of the way we handle endpoint/namespace support. An option would be to drop this middleware as well though.

Thinking about this more, I think we should keep the single argument signature because it's better for middleware support.

tofumatt · 2018-06-29T09:17:54Z

The API is fine to me, but I think using fetch is confusing. It's for API requests and calling it fetch just invites confusion to me. I think avoiding confusion with fetch is worth it (yes, I know the API is different but for newer JS developers this could be confusing, and it's also just a bit strange to scan–also folks might assume our fetch is a shim rather than a different API). Also calling it apiFetch makes the purpose of the package clearer.

youknowriad · 2018-06-29T09:25:16Z

I'm convinced :) updating.

nerrad · 2018-06-30T07:03:29Z

editor/store/effects.js

+	};
+	return response.json()
+		.then( getErrorFromBody )
+		.catch( () => getErrorFromBody() );


I wonder if it would be more useful to have this utility function within the apiFetch package itself?

nerrad · 2018-06-30T07:08:10Z

packages/api-fetch/README.md

+} );
+```
+
+The `api-fetch` package provides built-in middlewares you can use to provide a `nonce` and a custom `rootURL`.


Should the preloading and namespace-endpoint middlewares be mentioned here too?

Also, how does one leverage said nonce and rootURL middlewares ?

aduth

What do we do about https://www.npmjs.com/package/@wordpress/api-request ?

aduth · 2018-07-10T15:06:45Z

components/higher-order/with-api-data/request.js

+		.then( ( response ) => {
+			return response.json().then( ( body ) => ( {
+				body: body,
+				headers: response.headers,


What is the shape of response.headers here ? Is it an object implementing the Headers interface ? That doesn't seem compatible with how this was previously shaped as a set of tuples.

unless I'm missing something, I don't see it being used or exposed anywhere. I think it should be fine to remove entirely.

aduth · 2018-07-10T15:08:36Z

editor/components/post-taxonomies/flat-term-selector.js

+			method: 'POST',
+			data: { name: termName },
+		} ).catch( ( response ) =>
+			response.json().then( ( body ) => {


So in .then we receive a JSON payload by default ? But .catch we receive response always ? And in .then we have the option to receive response by passing parse: false ? Feels like an inconsistent developer experience.

Yes, what's the alternative you suggest? Try to parse the response in errors as well?

That, or just abandon the auto-parse convenience. Consistency could have the bigger benefit.

I personally prefer to keep the auto-parsing because of the duplication it can create. I'm fine with consistency between then/catch

I think parsing error response bodies makes sense since this package is specifically designed to work with the WP REST API, which we know returns nicely formatted error responses:

{ "code": "rest_forbidden", "data": { "status": 401 }, "message": "Sorry, you are not allowed to do that." }

Most error handling can then be as simple as:

apiFetch( ... ).catch( ( { message } ) => { console.error( message ); } );

If we cannot parse the error response body (e.g. because of a connection issue), then we should throw a similarly shaped object, e.g.:

{ code: "unknown_error", message: "An unknown error occurred.", }

aduth · 2018-07-10T15:11:54Z

packages/api-fetch/README.md

+} );
+```
+
+The `api-fetch` package provides built-in middlewares you can use to provide a `nonce` and a custom `rootURL`.


Also, how does one leverage said nonce and rootURL middlewares ?

aduth · 2018-07-10T15:15:08Z

packages/api-fetch/src/index.js

+		namespaceEndpointMiddleware,
+		httpV1Middleware,
+		raw,
+	].reverse();


I guess this is inherited, but why do we go through the effort to Array#reverse if we could already know what the result will be in assigning this array? i.e.

const steps = [ raw, httpV1Middleware, namespaceEndpointMiddleware, ...middlewares, ];

Addressed

noisysocks · 2018-07-16T01:06:58Z

packages/api-fetch/src/index.js

+			.then( parseResponse )
+			.catch( ( response ) => {
+				if ( ! parse ) {
+					Promise.reject( response );


We're missing a return here.

youknowriad · 2018-07-17T09:47:40Z

This should be ready to land if I can get another review :)

tofumatt

I think the helper functions used in the apiFetch promise chain should be defined outside the function and I think there are some little cleanup things that need to happen here.

tofumatt · 2018-07-17T10:12:43Z

editor/components/post-taxonomies/flat-term-selector.js

-				reject( xhr );
-			} );
+		const basePath = wp.api.getTaxonomyRoute( this.props.slug );
+		// Tries to create a term or fetch it if it already exists


Wild nitpick but comments should end in a period.

tofumatt · 2018-07-17T10:14:28Z

editor/components/post-taxonomies/flat-term-selector.js

+				this.addRequest = apiFetch( {
+					path: `/wp/v2/${ basePath }?${ stringify( { ...DEFAULT_QUERY, search: termName } ) }`,
+				} );
+				return this.addRequest.then( ( searchResult ) => {


I guess since we're returning from inside the catch call it's tough to chain this nicer but returning a promise's chained return value is a bit hard to parse/read/grok. Not sure how to make it nicer without async/await syntax which is a whole refactor though 🤷‍♂️

I find that using async/await is not necessarily better when we use .catch because you end up with await inside try/catch and you also have to embed try/catch inside try/catch sometimes.

tofumatt · 2018-07-17T10:14:46Z

editor/components/post-taxonomies/flat-term-selector.js

+		} ).catch( ( error ) => {
+			const errorCode = error.code;
+			if ( errorCode === 'term_exists' ) {
+				// search the new category created since last fetch


This should be a full sentence 😄

tofumatt · 2018-07-17T10:16:25Z

editor/store/effects.js

+		request
+			.then(
+				( newPost ) => {
+					const reset = isAutosave ? resetAutosave : resetPost;


This is pretty aggressive whitespace, any chance we could tighten it up a bit?

tofumatt · 2018-07-17T10:19:06Z

editor/store/test/effects.js

+							type: 'SAVE_SHARED_BLOCK_FAILURE',
+							id: 123,
+						} );
+						done();


Maybe I missed it in a previous review, but why the move to using done here over returning the Promise?

It was not possible on the initial implementation (I guess because of jQuery), I'll see if it's better now.

tofumatt · 2018-07-17T10:24:02Z

packages/api-fetch/src/index.js

+				headers,
+			}
+		);
+		const checkStatus = ( response ) => {


It seems odd to be that these are all defined as inline functions inside the function itself. Won't that mean they're instantiated every time the function is called? Why not have these all exist outside the function itself? They're already stateless/functional and it would improve readability of the function (you only have to follow the logic of the function over the definitions as well) and it might improve performance by avoiding re-creating them on each call.

parseResponse is not stateless, and I think creating inline function is not bad in terms of performance so I preferred consistency.

Oh shoot, I missed that.

That makes sense; I find inline functions make a function a lot tougher to understand right away, but that's cool.

tofumatt · 2018-07-17T10:25:10Z

packages/api-fetch/src/middlewares/preloading.js

@@ -32,15 +27,12 @@ const createPreloadingMiddleware = ( preloadedData ) => ( options, next ) => {
 			.join( '&' );
 	}

-	if ( typeof options.path === 'string' ) {
+	if ( typeof options.path === 'string' && options.parse !== false ) {


"Does not equal false" is harder for my brain (and probably other brains) to grok than "is true". Any reason not to just use options.parse or options.parse === true if you want to be strict about it?

The idea is that it's clearer this way that it's opt-out and not opt-in. The default value allows us to change the condition but I think it's a little bit safer to check against false values on "opt-out" flags.

Right, you need to pass parse: false to opt-out, that makes sense. But why not make the condition check if ( typeof options.path === 'string' && options.parse ) {?

I just find it easier to read that we're checking for true rather than for "not false".

👍 updating

Noting that if options.parse is undefined options.parse returns false but in the current condition it returns true so we have to assign a default value before simplifying the check.

Oh yeah, sorry, I should have said that; it was implied but I should've mentioned it explicitly. My bad 😞

I think that'll make the code easier to follow and the intent clearer as well. 👍

tofumatt · 2018-07-17T10:26:00Z

packages/api-fetch/src/middlewares/test/preloading.js

@@ -20,9 +20,8 @@ describe( 'Preloading Middleware', () => {
 		};

 		const response = prelooadingMiddleware( requestOptions );
-		response.done( ( value ) => {
+		return response.then( ( value ) => {


And here you moved to returning the Promise, so I'm not sure why you went away from it before 😄

tofumatt

Makes sense! I left a note about options.parse just because I think the conditional is weird and I'm still not huge on inline functions for readability. I'd be curious about the performance difference for such a frequently-used function. 🤔

Anyway, looks good!

tofumatt · 2018-07-17T11:01:56Z

packages/api-fetch/src/index.js

+				headers,
+			}
+		);
+		const checkStatus = ( response ) => {


Oh shoot, I missed that.

That makes sense; I find inline functions make a function a lot tougher to understand right away, but that's cool.

tofumatt · 2018-07-17T11:03:38Z

packages/api-fetch/src/middlewares/preloading.js

@@ -32,15 +27,12 @@ const createPreloadingMiddleware = ( preloadedData ) => ( options, next ) => {
 			.join( '&' );
 	}

-	if ( typeof options.path === 'string' ) {
+	if ( typeof options.path === 'string' && options.parse !== false ) {


Right, you need to pass parse: false to opt-out, that makes sense. But why not make the condition check if ( typeof options.path === 'string' && options.parse ) {?

I just find it easier to read that we're checking for true rather than for "not false".

youknowriad · 2018-07-17T11:53:00Z

Thanks all for the reviews.

youknowriad · 2018-07-17T11:53:49Z

What do we do about https://www.npmjs.com/package/@wordpress/api-request ?

We should probably just add a "deprecated" notice manually to the npm README

gziolo · 2018-07-17T13:56:00Z

@ntwb any hints, how we should proceed?

aduth · 2018-07-17T14:32:42Z

Running npm run docs:build on master after this merge produces some local changes:

diff --git a/docs/manifest.json b/docs/manifest.json
index 3efafeb88..66ae6255e 100644
--- a/docs/manifest.json
+++ b/docs/manifest.json
@@ -222,9 +222,9 @@
 		"parent": "packages"
 	},
 	{
-		"title": "@wordpress/api-request",
-		"slug": "packages-api-request",
-		"markdown_source": "https://github.com/raw/WordPress/gutenberg/master/packages/api-request/README.md",
+		"title": "@wordpress/api-fetch",
+		"slug": "packages-api-fetch",
+		"markdown_source": "https://github.com/raw/WordPress/gutenberg/master/packages/api-fetch/README.md",
 		"parent": "packages"
 	},
 	{

youknowriad · 2018-07-17T14:52:34Z

good catch @aduth forgot this one :)

ntwb · 2018-07-18T01:01:39Z

What do we do about npmjs.com/package/@wordpress/api-request ?

We should probably just add a "deprecated" notice manually to the npm README

@ntwb any hints, how we should proceed?

I've just deprecated @wordpress/api-request on npmjs.com

If someone installs it they will see a notice:

"This package is deprecated, use @wordpress/api-fetch instead."

ntwb · 2018-07-18T01:02:13Z

cc @gziolo @youknowriad ☝️

gziolo · 2018-07-18T06:20:57Z

Awesome, thanks!

Starting in Gutenberg 3.1, the 'wp-api-request' script gets de-registered and re-registered with a Gutenberg-specific version. Unfortunately, the script doesn't also get re-localized with the `wpApiSettings` variable that's needed for wp-api.js, unless the current screen contains an editor instance. The bug was reported here: WordPress/gutenberg#7488 And it should be fixed once the changes in this pull request get released: WordPress/gutenberg#7566 In the meantime, this hotfix should ensure that `wpApiSettings` is available regardless of the presence of an editor instance. git-svn-id: https://meta.svn.wordpress.org/sites/trunk/wordcamp.org/public_html/wp-content/mu-plugins@7476 74240141-8908-4e6f-9713-ba540dce6ec7

aduth · 2018-08-03T13:33:52Z

packages/api-fetch/README.md

@@ -0,0 +1,64 @@
+# @wordpress/api-fetch
+
+Wrapper around `window.fetch` to call WordPress REST APIs.


Minor: Should we maybe be more explicit for general use that this requires Fetch (and thus, a polyfill maybe required for older browsers?)

yes, definitely, this makes me think we may need a consistent way to gather the required polyfills for each package. Better automated but It's probably not easy to automate.

Related: #7159

Starting in Gutenberg 3.1, the 'wp-api-request' script gets de-registered and re-registered with a Gutenberg-specific version. Unfortunately, the script doesn't also get re-localized with the `wpApiSettings` variable that's needed for wp-api.js, unless the current screen contains an editor instance. The bug was reported here: WordPress/gutenberg#7488 And it should be fixed once the changes in this pull request get released: WordPress/gutenberg#7566 In the meantime, this hotfix should ensure that `wpApiSettings` is available regardless of the presence of an editor instance. git-svn-id: https://meta.svn.wordpress.org/sites/trunk/wordcamp.org@7476 74240141-8908-4e6f-9713-ba540dce6ec7

Starting in Gutenberg 3.1, the 'wp-api-request' script gets de-registered and re-registered with a Gutenberg-specific version. Unfortunately, the script doesn't also get re-localized with the `wpApiSettings` variable that's needed for wp-api.js, unless the current screen contains an editor instance. The bug was reported here: WordPress/gutenberg#7488 And it should be fixed once the changes in this pull request get released: WordPress/gutenberg#7566 In the meantime, this hotfix should ensure that `wpApiSettings` is available regardless of the presence of an editor instance. git-svn-id: https://meta.svn.wordpress.org/sites/trunk/wordcamp.org@7476 74240141-8908-4e6f-9713-ba540dce6ec7 git-svn-id: https://github.com/WordPress/wordcamp.org.git@1266 a64dc54a-4538-25e2-d9ce-ce57b7db7bff

Starting in Gutenberg 3.1, the 'wp-api-request' script gets de-registered and re-registered with a Gutenberg-specific version. Unfortunately, the script doesn't also get re-localized with the `wpApiSettings` variable that's needed for wp-api.js, unless the current screen contains an editor instance. The bug was reported here: WordPress/gutenberg#7488 And it should be fixed once the changes in this pull request get released: WordPress/gutenberg#7566 In the meantime, this hotfix should ensure that `wpApiSettings` is available regardless of the presence of an editor instance. git-svn-id: http://meta.svn.wordpress.org/sites/trunk/wordcamp.org@7476 74240141-8908-4e6f-9713-ba540dce6ec7

Starting in Gutenberg 3.1, the 'wp-api-request' script gets de-registered and re-registered with a Gutenberg-specific version. Unfortunately, the script doesn't also get re-localized with the `wpApiSettings` variable that's needed for wp-api.js, unless the current screen contains an editor instance. The bug was reported here: WordPress/gutenberg#7488 And it should be fixed once the changes in this pull request get released: WordPress/gutenberg#7566 In the meantime, this hotfix should ensure that `wpApiSettings` is available regardless of the presence of an editor instance. git-svn-id: https://meta.svn.wordpress.org/sites/trunk/wordcamp.org@7476 74240141-8908-4e6f-9713-ba540dce6ec7

youknowriad added the npm Packages Related to npm packages label Jun 27, 2018

youknowriad self-assigned this Jun 27, 2018

youknowriad mentioned this pull request Jun 27, 2018

Gutenberg shouldn't force all Plugins to use its wp-api-request override #7329

Closed

4 tasks

youknowriad force-pushed the try/fetch-package branch from 30247fa to ea90c0d Compare June 28, 2018 11:12

youknowriad changed the title ~~WIP: Packages: Replace the apiRequest module with fetch module~~ Packages: Replace the apiRequest module with fetch module Jun 28, 2018

youknowriad requested a review from a team June 28, 2018 12:30

youknowriad force-pushed the try/fetch-package branch from 717c018 to 19cd0db Compare June 28, 2018 13:28

joyously reviewed Jun 28, 2018

View reviewed changes

noisysocks previously requested changes Jun 29, 2018

View reviewed changes

youknowriad changed the title ~~Packages: Replace the apiRequest module with fetch module~~ Packages: Replace the apiRequest module with api-fetch module Jun 29, 2018

nerrad reviewed Jun 30, 2018

View reviewed changes

aduth reviewed Jul 10, 2018

View reviewed changes

youknowriad force-pushed the try/fetch-package branch from d2b1f58 to e7bff50 Compare July 13, 2018 11:27

youknowriad requested a review from gziolo July 13, 2018 11:43

noisysocks reviewed Jul 16, 2018

View reviewed changes

youknowriad force-pushed the try/fetch-package branch 2 times, most recently from 8783df9 to 0fd300e Compare July 17, 2018 09:47

tofumatt requested changes Jul 17, 2018

View reviewed changes

youknowriad requested a review from tofumatt July 17, 2018 10:50

tofumatt approved these changes Jul 17, 2018

View reviewed changes

youknowriad added 2 commits July 17, 2018 12:41

Packages: Replace the apiRequest module with fetch module

99bba80

Fix content type for JSON requests

0987a59

youknowriad added 6 commits July 17, 2018 12:42

Avoid the reverse call

9d9dfdb

Expanding the middlewares documentation

bec4367

Small documentation tweaks

b72caf2

Return promises instead of using done in tests

c9e12b1

Fix indentation

7feb036

Simplify parse condition for readability

6b96317

youknowriad force-pushed the try/fetch-package branch from 1e9b4e8 to 6b96317 Compare July 17, 2018 11:42

youknowriad merged commit 247a6e0 into master Jul 17, 2018

youknowriad deleted the try/fetch-package branch July 17, 2018 11:52

youknowriad mentioned this pull request Jul 17, 2018

Api Fetch: Fix metaboxes save request (parse: false) #8002

Merged

aduth mentioned this pull request Jul 18, 2018

Components: Error when URLInput updates its suggestions #8034

Closed

gwwar mentioned this pull request Jul 19, 2018

Proof of concept: render a Gutenberg component in Calypso DevDocs Automattic/wp-calypso#26088

Closed

4 tasks

aduth reviewed Aug 3, 2018

View reviewed changes

pento mentioned this pull request Sep 16, 2018

Error when trying to use @wordpress/api-request package standalone #9935

Closed

t-hamano mentioned this pull request Jan 3, 2023

@wordpress/apiFetch fails remote site requests. #46787

Open

		@@ -0,0 +1,64 @@
		# @wordpress/api-fetch

		Wrapper around `window.fetch` to call WordPress REST APIs.

Packages: Replace the apiRequest module with api-fetch module #7566

Packages: Replace the apiRequest module with api-fetch module #7566

Conversation

youknowriad commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jun 29, 2018

tofumatt commented Jun 29, 2018

youknowriad commented Jun 29, 2018 • edited Loading

tofumatt commented Jun 29, 2018

youknowriad commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jul 17, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jul 17, 2018

youknowriad commented Jul 17, 2018

gziolo commented Jul 17, 2018

aduth commented Jul 17, 2018

youknowriad commented Jul 17, 2018

ntwb commented Jul 18, 2018

ntwb commented Jul 18, 2018

gziolo commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jun 27, 2018 •

edited

Loading

youknowriad commented Jun 29, 2018 •

edited

Loading