Add a new `@wordpress/api-request` package #7018

youknowriad · 2018-05-30T12:27:44Z

This PR creates a new package @wordpress/api-request

The implementation is based on the Core api-request package (backwards compatible)
Instead of relying on globals to set the nonce/rootURL, it users configurable middlewares
The preloading support is also built as a middleware.

I don't know about you but I love this PR :P

Testing instructions

Check that Gutenberg loads posts and save them as expected.

nerrad

I'm really excited about the direction of moving wp global pieces into packages. However, I'm thinking it might be good to create a corresponding trac ticket so there's visibility there as well and also ensures that's being tracked for eventual merge from here?

nerrad · 2018-05-30T12:32:49Z

packages/api-request/src/index.js

+ */
+import jQuery from 'jquery';
+
+const wpApiSettings = window.wpApiSettings;


Would there be value in having a package that's dedicated to all options/settings etc passed through from the server? I'm thinking then you'd just do something like:

import { API_SETTINGS } from '@wordpress/settings';

This is a leftover, I killed this global :)

Aside, I don't think a settings package would be a good thing, these settings are server-side data. An endpoint for these would be good, but until then. Providing config using wp_add_inline_script calls is the way to go to keep the packages generic.

OOh you're reviewing the first commit only ;)

lol ya I jumped on a bit early ha!

youknowriad · 2018-06-05T10:33:45Z

I'm not certain I understand why the e2e tests are completely failing in this PR. Locally they work, can someone take a look?

gziolo

Nice work, I left some comments. I will try to run e2e test locally and is if they work.

gziolo · 2018-06-05T10:47:15Z

.eslintrc.js

@@ -26,7 +26,7 @@ module.exports = {
 		'jest/globals': true,
 	},
 	globals: {
-		wpApiSettings: true,
+		wpApiSchema: true,


Is there any way to avoid using this global?

Not at the moment, I believe the need for this will go away once we refactor our code to move away from withApiData

Right, thanks for making it clear :)

gziolo · 2018-06-05T10:52:16Z

packages/api-request/src/middlewares/root-url.js

@@ -0,0 +1,34 @@
+import namespaceAndEndpointMiddleware from './namespace-endpoint';


Nit: missing Internal dependencies comment

gziolo · 2018-06-05T10:52:30Z

packages/api-request/src/middlewares/test/http-v1.js

@@ -0,0 +1,21 @@
+import httpV1Middleware from '../http-v1';


Nit: missing Internal dependencies comment

gziolo · 2018-06-05T10:53:09Z

packages/api-request/src/middlewares/test/namespace-endpoint.js

@@ -0,0 +1,18 @@
+import namespaceEndpointMiddleware from '../namespace-endpoint';


Nit: missing Internal dependencies comment

gziolo · 2018-06-05T10:55:46Z

packages/api-request/src/middlewares/test/namespace-endpoint.js

+			expect( options.endpoint ).toBeUndefined();
+		};
+
+		namespaceEndpointMiddleware( requestOptions, callback );


We should add expect.hasAssertions call to make sure that callback was executed.

gziolo · 2018-06-05T10:56:12Z

packages/api-request/src/middlewares/test/http-v1.js

+			expect( options ).toEqual( requestOptions );
+		};
+
+		httpV1Middleware( requestOptions, callback );


We should add expect.hasAssertions call to both tests to make sure that callback was executed.

gziolo · 2018-06-05T10:57:12Z

packages/api-request/src/middlewares/test/nonce.js

+			expect( options.headers[ 'X-WP-Nonce' ] ).toBe( 'existing nonce' );
+		};
+
+		nonceMiddleware( requestOptions, callback );


We should add expect.hasAssertions call to both tests to make sure that callback was executed.

gziolo · 2018-06-05T10:58:04Z

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

+		};
+
+		const response = prelooadingMiddleware( requestOptions );
+		response.then( ( value ) => {


I think returning this statement will ensure that Promise got completed.

This is not a regular promise (jQuery), so I went with the done trick

gziolo · 2018-06-05T10:58:18Z

packages/api-request/src/middlewares/test/root-url.js

+			expect( options.url ).toBe( 'http://wp.org/wp-admin/rest/wp/v2/posts' );
+		};
+
+		rootURLMiddleware( requestOptions, callback );


We should add expect.hasAssertions call to make sure that callback was executed.

gziolo · 2018-06-05T12:23:52Z

I'm not certain I understand why the e2e tests are completely failing in this PR. Locally they work, can someone take a look?

I was able to reproduce it locally by running npm run build, I can see the issue in both browser and when triggerring e2e tests. I think there is some issue with configuration because HTML file contains tons of JSON data.

youknowriad · 2018-06-05T12:31:04Z

I was able to reproduce it locally by running npm run build

npm run build works properly for me 🤔 Maybe related to the environment

brandonpayton · 2018-06-05T15:46:23Z

I took a look at this and am seeing a large blank space where the editor should be. In case it helps, I noticed a difference between this branch and master when executing the following line:
https://github.com/WordPress/WordPress/blob/6fd9d2edcf488bdc4493e388a5b7cba025f8f68f/wp-includes/js/wp-api.js#L1516

On master, attributes.schema is populated, but with this branch, attributes.schema is null.

youknowriad · 2018-06-06T10:46:50Z

Thanks for the hint @brandonpayton, so it looks like the wp-api WP script relies on globals defined by wp-api-request which I'm replacing in this PR. What I did is that I moved the localisation of these globals to wp-api.

On another topic, I tried to shim jQuery.deferred and return a regular Promise. It's way more complex than we though and I don't think it's achievable prope rly.

The .then method of the deffered takes the body as the first parameters but also takes other arguments (status, xhr) while if we try to match fetch, we only return the body in the first one and we can't detect that these arguments are being used to show a warning or something.

Also, if we want to provide a way forward for people using xhr (we do so in our code as well), we need to provide the full xhr object here but it should match the fetch's response object and it's very difficult to convert from xhr to fetch (remember we can't just use fetch yet because we need BC for other methods requiring xhr).

I think we should merge this PR as is and tackle the deprecated in follow-ups. I also think It's not possible to accomodate both APIs at the same time. the best way to do it is to use another name for wp.apiRequest or introduce the versionned globals or similar.

cc @aduth

gziolo · 2018-06-07T12:53:27Z

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

 	beforeEach( () => {
 		getStablePath.clear();
 		for ( const key in cache ) {
 			delete cache[ key ];
 		}

-		wpApiRequest = wp.apiRequest;
-		wp.apiRequest = jest.fn( () => ( {
+		apiRequest.mockReturnValue = {


This part becomes a bit magic since this module is being mocked in the setup file. Have you tried to use Manual Mocks using __mocks__ folder? If you do so you need to explicitly call jest.mock('./moduleName') in your test file which is easier to follow.

I tried jest.mock('./moduleName', () => {}) but It didn't work. I'll take a look at the manual mocks.

What seems to work for me is I put a __mocks__/moduleName within the same path as the test (so test/__mocks__/moduleName/index.js) and the mock is in the file then anything importing that module being tested will use the mock instead. Seems to work well for me.

Not that important at the moment, to be honest. Just wanted to make sure you are aware of other options. We can refactor later.

gziolo

I checked generated HTML for the page, it looks like wpApiSettings.schema and wp.apiRequest.createPreloadingMiddleware contain the same data. We probably should only use one of them.

gziolo · 2018-06-07T13:43:36Z

packages/api-request/package.json

+	"main": "build/index.js",
+	"module": "build-module/index.js",
+	"dependencies": {
+		"jquery": "^3.3.1"


We use 3.2.1 version in the top level package.json. We should align those two or Lerna will install another copy in the package subfolder.

gziolo · 2018-06-07T13:58:40Z

lib/client-assets.php

+	wp_localize_script( 'wp-api', 'wpApiSettings', array(
+		'root'          => esc_url_raw( get_rest_url() ),
+		'nonce'         => ( wp_installing() && ! is_multisite() ) ? '' : wp_create_nonce( 'wp_rest' ),
+		'versionString' => 'wp/v2/',


Do we ever use it? I think it might be useful, but I don't see any code occurrence inside Gutenberg.

This global is the reason the tests were failing, I just left it as is, it's being used by Core's wp-api

@gziolo

Thanks for the feedback @gziolo we already have an issue tracking this double loading of the schema #6539 It's kind of unrelated to this refactoring

gziolo · 2018-06-07T16:13:03Z

I checked generated HTML for the page, it looks like wpApiSettings.schema and wp.apiRequest.createPreloadingMiddleware contain the same data. We probably should only use one of them.

Yes, it looks like it is a known issue: #6539. Let's address it separately. I wasn't aware it was already reported. In that case, everything works well in my testing.

gziolo

Travis has some issues. I cleared cache and triggered another build. It works locally as expected, e2e tests pass to. Let's make sure Travis is happy and it should be good to go 🚢

aduth · 2018-10-31T20:34:42Z

packages/api-request/src/middlewares/http-v1.js

@@ -0,0 +1,19 @@
+function httpV1Middleware( options, next ) {
+	const newOptions = { ...options };


Why do we create a shallow clone before the if condition where it becomes relevant? Wasted effort for the majority of cases?

aduth · 2019-01-23T20:28:21Z

packages/api-request/src/middlewares/nonce.js

+const createNonceMiddleware = ( nonce ) => ( options, next ) => {
+	let usedNonce = nonce;
+	/**
+	 * This is not ideal but it's fine for now.


This comment seems to imply some self-evident flaw. What's not ideal? What's the action item for some future maintainer to make it better?

It didn't feel right to me to tie apiRequest/fetch to heartbeat specific events. I think it's better in the last versions using hooks. I don't have an ideal solution without complexity though.

Looking at it some more, I think I could see a couple things:

There's an X-WP-Nonce on the response of a fetch request; should we not assume that as a fresh one and use it?

Maybe this (the heartbeat tick handling) should be pulled out and added as an inline script in the WordPress-specific usage of api-request, with some way to inform the middleware of a new nonce.

Both these suggestion seem good to me, I just worry about the complexity especially about this:

with some way to inform the middleware of a new nonce.

If we manage to keep it simple in terms of API and implementation, all good for me.

One idea: optionally allow, instead of an initial nonce, a function which receives as an argument the "inform" callback:

createNonceMiddleware( ( setNonce ) => { setNonce( initialNonce ); addAction( 'heartbeat.tick', 'core/api-fetch/create-nonce-middleware', ( response ) => response[ 'rest_nonce' ] && setNonce( response[ 'rest_nonce' ] ), ); } );

Lazy man's observable.

Or, just have the nonce as a property of the created middleware.

const nonceMiddleware = createNonceMiddleware( initialNonce ); wp.apiFetch.use( nonceMiddleware ); wp.hooks.addAction( 'heartbeat.tick', 'core/api-fetch/create-nonce-middleware', ( response ) => response[ 'rest_nonce' ] && nonceMiddleware.nonce = response[ 'rest_nonce' ] );

With that in mind, should there be an optional callback that is passed in that can be used to update the nonce when its observed in the response of a fetch?

What's the use-case?

I was thinking there might be some global reference to the current nonce but I'm guessing there's not (and doesn't need to be obviously).

I think of the options given, I prefer the callback approach. Exposing nonce as a property on nonceMiddleware seems potentially fragile because third party code could clobber it right?

They'd need a reference to the created middleware (not likely to be exposed). Could even be argued to be a good thing to allow someone to update it themselves if they want, though. I don't see it likely to be an issue.

youknowriad added the npm Packages Related to npm packages label May 30, 2018

youknowriad self-assigned this May 30, 2018

youknowriad requested a review from a team May 30, 2018 12:27

nerrad reviewed May 30, 2018

View reviewed changes

youknowriad force-pushed the add/api-request-package branch from b4a2563 to caa9ae9 Compare June 1, 2018 11:06

youknowriad mentioned this pull request Jun 2, 2018

Fix getWPAdminUrl function to actually return the /wp-admin/ URL #7096

Closed

youknowriad force-pushed the add/api-request-package branch from da95f14 to 395286e Compare June 4, 2018 11:58

gziolo reviewed Jun 5, 2018

View reviewed changes

youknowriad force-pushed the add/api-request-package branch 2 times, most recently from 73114d2 to 5fd5112 Compare June 6, 2018 09:24

youknowriad added 8 commits June 7, 2018 10:25

Add a wordpress/api-request package

3433740

Add the middlewares API

99b9907

Fix unit tests

5ce2996

Adding unit tests

304a323

Adding internal dependencies comment

60fa651

Improve unit tests

77a8220

Restore wpApiSettings global for wp-api

c6b3744

Lint package.json

3767199

youknowriad force-pushed the add/api-request-package branch from df19150 to 3767199 Compare June 7, 2018 09:25

gziolo reviewed Jun 7, 2018

View reviewed changes

gziolo previously requested changes Jun 7, 2018

View reviewed changes

Align jQuery versions

0724e4e

gziolo approved these changes Jun 7, 2018

View reviewed changes

youknowriad merged commit 8ec748a into master Jun 8, 2018

youknowriad deleted the add/api-request-package branch June 8, 2018 08:36

Shelob9 mentioned this pull request Jun 8, 2018

Packages: Publish all modules as independent npm packages #3955

Closed

19 tasks

mtias added this to the 3.1 milestone Jun 20, 2018

aduth reviewed Oct 31, 2018

View reviewed changes

aduth mentioned this pull request Oct 31, 2018

API Fetch: Avoid unnecessary options shallow clone on unhandled method #11332

Merged

aduth reviewed Jan 23, 2019

View reviewed changes

		@@ -0,0 +1,34 @@
		import namespaceAndEndpointMiddleware from './namespace-endpoint';

		@@ -0,0 +1,18 @@
		import namespaceEndpointMiddleware from '../namespace-endpoint';

		@@ -0,0 +1,19 @@
		function httpV1Middleware( options, next ) {
		const newOptions = { ...options };

Add a new @wordpress/api-request package #7018

Add a new @wordpress/api-request package #7018

Conversation

youknowriad commented May 30, 2018 • edited Loading

nerrad 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

youknowriad commented Jun 5, 2018

gziolo 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

gziolo commented Jun 5, 2018

youknowriad commented Jun 5, 2018

brandonpayton commented Jun 5, 2018

youknowriad commented Jun 6, 2018 • edited Loading

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

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jun 7, 2018

gziolo 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

Add a new `@wordpress/api-request` package #7018

Add a new `@wordpress/api-request` package #7018

youknowriad commented May 30, 2018 •

edited

Loading

youknowriad commented Jun 6, 2018 •

edited

Loading

gziolo left a comment •

edited

Loading

youknowriad Jun 7, 2018 •

edited

Loading