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

API Request: Add preloading support to wp.apiRequest #6076

Merged
merged 5 commits into from Apr 11, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Apr 9, 2018

Since we're moving more stuff to use the Data Module instead of withAPIData we need a way to prefetch data the same way we used to do with withAPIData. This PR adds preloading support using the same mechanism used in withAPIData but built-in into the low level apiRequest.

I'm not removing the caching support from withAPIData yet because it has more features, it also caches new requests as they're performed, and this is not needed in the Data Module since the data module is a cache in itself.

Testing instructions

  • Check that when loading Gutenberg for a new post, there's no XHR request triggered. Specifically the /wp/v2/types/post?context=edit request.

@youknowriad youknowriad self-assigned this Apr 9, 2018

@youknowriad youknowriad requested a review from aduth Apr 9, 2018

return Promise.resolve( window._wpAPIDataPreload[ path ].body );
}
return previousApiRequest(request);

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

Minor: Style: Space between parenthesis.

return Promise.resolve( window._wpAPIDataPreload[ path ].body );
}
return previousApiRequest(request);

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

Minor: We lose the this context calling this way. wp.apiRequest doesn't use it currently, but doesn't mean it couldn't.

Maybe instead:

return previousApiRequest.call( previousApiRequest, request );

This comment has been minimized.

@youknowriad

youknowriad Apr 9, 2018

Contributor

I believe depending on the way it's defined the original context could be different than previousApiRequest (it could be wp for instance)

// [ [ 'b, '1' ], [ 'c', '2' ], [ 'a', '5' ] ]
.map( ( entry ) => entry.split( '=' ) )
// [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ]
.sort( ( a, b ) => a[ 0 ].localeCompare( b[ 0 ] ) )

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

This will error in IE11 (arrow functions).

var method = request.method || 'GET';
var path = getStablePath( request.path )
if ( 'GET' === method && window._wpAPIDataPreload[ path ] ) {
return Promise.resolve( window._wpAPIDataPreload[ path ].body );

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

The original wp.apiRequest returns a jQuery.Deferred, meaning a consumer may expect to be able to call .done, which won't work with Promise.resolve.

Should be better now :)

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 11, 2018

Nit: gutenberg_shim_fix_api_request_plain_permalinks - doesn't seem to be the best name for the surrounding function :)

I tested in the browser some more advanced use case and it seems to work as expected:

getStablePath( 'https://github.com/WordPress/gutenberg/pull/6085?list_d=&list_c=3&list_c=2&list_c=1&list_b[]=3&list_b[]=2&list_b[]=1&list_a=3,2,1' );

"https://github.com/WordPress/gutenberg/pull/6085?list_a=3,2,1&list_b[]=3&list_b[]=2&list_b[]=1&list_c=3&list_c=2&list_c=1&list_d="

Inspired by https://stackoverflow.com/questions/6243051/how-to-pass-an-array-within-a-query-string :)

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 11, 2018

I can see that data was preloaded:

screen shot 2018-04-11 at 09 35 09

but I still see the request:

screen shot 2018-04-11 at 09 35 55

wp-json prefix?

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 11, 2018

Some more details I was able to collect when debugging. I updated the code as follows:

    var buildAjaxOptions;

    console.log( wpApiSettings.root );
    if ( 'string' !== typeof wpApiSettings.root ||
            -1 === wpApiSettings.root.indexOf( '?' ) ) {
        console.log( 'bailing out early' );
        return;
    }

This is what I could observe on the JS console:
screen shot 2018-04-11 at 12 41 17

It looks like there are 2 conflicting requirements for both shims, which only confirms that my previous concern about gutenberg_shim_fix_api_request_plain_permalinks was valid.

var previousApiRequest = wp.apiRequest;
wp.apiRequest = function( request ) {
var method = request.method || 'GET';
var path = getStablePath( request.path )

This comment has been minimized.

@gziolo

gziolo Apr 11, 2018

Member

Nit: Missing semicolon 😱

return previousApiRequest.call( previousApiRequest, request );
}
wp.apiRequest.transport = previousApiRequest.transport;

This comment has been minimized.

@gziolo

gziolo Apr 11, 2018

Member

Should this iterate using for (var name in previousApiRequest) with hasOwnProperty check to be sure we don't remove any randomly added property by another plugin?

This comment has been minimized.

@youknowriad

youknowriad Apr 11, 2018

Contributor

Nice suggestion, updated. Thanks

@gziolo

gziolo approved these changes Apr 11, 2018

I tested it locally and it works properly. I had still one concern, but I don't know if that is even an issue. I'm starting to be suspicious about our assumptions because of the nature of the changes we introduce ourselves :)

@youknowriad youknowriad merged commit 8fa91e6 into master Apr 11, 2018

2 checks passed

codecov/project 44.53% remains the same compared to 1f643e0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/preload-support-to-wp-apirequest branch Apr 11, 2018

@aduth

This comment has been minimized.

Member

aduth commented Apr 11, 2018

This breaks saving when a meta box is present, because the meta box saves calling wp.apiRequest with url, not path:

url: window._wpMetaBoxUrl,

So this line throws an error:

var splitted = path.split( '?' );

Should be simple enough to fix, though will also look into if we can easily add an E2E test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment