Skip to content
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

getEntityRecords for wp_template returns null the first time it runs. #36380

Closed
Addison-Stavlo opened this issue Nov 10, 2021 · 19 comments
Closed
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@Addison-Stavlo
Copy link
Contributor

Description

This seems to be a recent regression that I can not reproduce on 11.8, but is present in 11.9 RCs and current trunk branch.

Core getEntityRecords selector for wp_templates returns null the first time it runs after loading the editor

Step-by-step reproduction instructions

  1. Load the Site Editor
  2. Open the console window and run the following: wp.data.select( 'core' ).getEntityRecords( 'postType', 'wp_template', { per_page: -1, } )
  3. Notice it returns null as the result.
  4. Run this a second time and the return should be the expected array of templates.

Screenshots, screen recording, code snippet

Screen Shot 2021-11-10 at 1 47 27 PM

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@Addison-Stavlo Addison-Stavlo added this to the Gutenberg 11.9 milestone Nov 10, 2021
@Addison-Stavlo Addison-Stavlo added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Nov 10, 2021
@youknowriad
Copy link
Contributor

Hi there! this is basically the behavior of any async selector. The first time the selector returns null and triggers a resolver. What might have changed is that the selector might not be preloaded anymore. And If I'm not wrong "templates" don't support pagination, so you should drop the { per_page: -1, } from the call.

@fullofcaffeine
Copy link
Member

The first time the selector returns null and triggers a resolver. What might have changed is that the selector might not be preloaded anymore

Thanks for chiming in, Riad! Do we know why that's the case now? It seems counter-intuitive to return null without a way to await the actual fetched data.

@shadigaafar
Copy link

I have notice that before, this is way sometimes I don't use it. And prefer to use apiFetch when I have something to do after loading. sometimes you do want to await for the fetched data. and it is not possible with getEntityRecords or getEntityRecord.

@Addison-Stavlo
Copy link
Contributor Author

What might have changed is that the selector might not be preloaded anymore.

That makes sense. 👍

@ockham
Copy link
Contributor

ockham commented Nov 10, 2021

The relevant preloading happens here:

'preload_paths' => array_merge(
gutenberg_get_navigation_areas_paths_to_preload(),
array(
array( '/wp/v2/media', 'OPTIONS' ),
'/',
'/wp/v2/types?context=edit',
'/wp/v2/taxonomies?context=edit',
'/wp/v2/pages?context=edit',
'/wp/v2/categories?context=edit',
'/wp/v2/posts?context=edit',
'/wp/v2/tags?context=edit',
'/wp/v2/templates?context=edit',
'/wp/v2/template-parts?context=edit',
'/wp/v2/settings',
'/wp/v2/themes?context=edit&status=active',
'/wp/v2/global-styles/' . $active_global_styles_id . '?context=edit',
'/wp/v2/global-styles/' . $active_global_styles_id,
'/wp/v2/themes/' . $active_theme . '/global-styles',
)
),

The templates endpoint (which I think is the one in charge of the wp_template CPT) is preloaded:

'/wp/v2/templates?context=edit',

So it's a bit surprising that getEntityRecords() doesn't pick that up 🤔

@ockham
Copy link
Contributor

ockham commented Nov 10, 2021

To elaborate a bit more, preloading means that on the PHP side, we list REST API endpoints whose responses we want to make available upon first loading a given page (such as the site editor). WordPress will then embed those responses into the server-generated HTML, and make them available to the JS on the client side.

To inspect, navigate to the site editor, switch to "View Source", and Cmd+F createPreloadingMiddleware.

@ockham
Copy link
Contributor

ockham commented Nov 10, 2021

To inspect, navigate to the site editor, switch to "View Source", and Cmd+F createPreloadingMiddleware.

FWIW, if I do that, I do see preloaded data for /wp/v2/templates?context=edit. So it seems like the issue might be on the client side (e.g. apiFetch or createPreloadingMiddleware). Since the core-data stack is somewhat big, it's a bit hard to guess where exactly.

Based on Addie's description, this might be a good candidate for bisecting:

This seems to be a recent regression that I can not reproduce on 11.8, but is present in 11.9 RCs and current trunk branch.

cc/ @fullofcaffeine since you're experienced with using bisect to track down issues like this 😬

@ockham
Copy link
Contributor

ockham commented Nov 10, 2021

Dug a bit and found a PR that could be related: #36044

@youknowriad
Copy link
Contributor

Yes, I think what I did on that PR is to remove the per_page argument from Gutenberg's code base and changed the preloaded URLs based on the API calls that I saw being called when first loading the site editor.

@ockham
Copy link
Contributor

ockham commented Nov 11, 2021

Yes, I think what I did on that PR is to remove the per_page argument from Gutenberg's code base and changed the preloaded URLs based on the API calls that I saw being called when first loading the site editor.

FWIW, I did try wp.data.select( 'core' ).getEntityRecords( 'postType', 'wp_template' ) (without the per_page), and it showed the same behavior (null on first invocation), so I think it's not just that 🤔

@fullofcaffeine
Copy link
Member

@fullofcaffeine since you're experienced with using bisect to track down issues like this

I'll have a look today 👀

@fullofcaffeine
Copy link
Member

fullofcaffeine commented Nov 12, 2021

@ockham Bisect points me to #35802. You can verify by:

  1. git checkout 630db5dcabe65dac3cc13808547a520a6d40853c
  2. npm ci && npm run build && npm run wp-env start .
  3. Open a new post and then the JS console, run this: wp.data.select( 'core' ).getEntityRecords( 'postType', 'wp_template', { per_page: -1, } ). Verify that the return value is null.
  4. git revert 630db5dcabe65dac3cc13808547a520a6d40853c
  5. npm ci && npm run build && npm run wp-env start .
  6. Open a new post and then the JS console, run this: wp.data.select( 'core' ).getEntityRecords( 'postType', 'wp_template', { per_page: -1, } ). Verify that the return value is an Array.

cc @Addison-Stavlo @youknowriad @Mamaduka

@Mamaduka
Copy link
Member

Thanks for the ping, @fullofcaffeine.

It's weird that #35802 affected pre-loading at all. PR only adds an extra argument to the REST API endpoint.

I'm looking at the issue right now and will post my findings shortly.

@Mamaduka
Copy link
Member

I did more testings, and I don't think this is a regression. You'll get similar results if you try to fetch pages in Site Editor and this path is also preloaded - wp.data.select( 'core' ).getEntityRecords( 'postType', 'page' ).

What has changed

Gutenberg 11.9 removed NavigationPanel, which was using the same selector, and any additional calls to getEntityRecords( 'postType', 'wp_template', { per_page: -1, } ) would have returned cached data from store.

Why pre-loaded paths return null on the first invocation

  • Resolver doesn't know about preloaded paths ( preloadedPaths !== preloadedState ). It makes request if data isn't available in the state and returns null
  • The createPreloadingMiddleware intercepts this request, and if the path matches, it returns pre-loaded data.
  • Items are received and stored in state.
  • Second call to selector return these items.

P.S. I'm not as familiar with core data as others, so this is my observation about its functionality.

@Mamaduka
Copy link
Member

Hi, @Addison-Stavlo

I just wanted to check in to see if we can close this issue?

@Addison-Stavlo
Copy link
Contributor Author

Certainly! If the consensus is that this is not a regression, feel free to close it. I noticed it had changed, but didn't realize it was expected behavior since the nav panel was removed. Thanks for the explanation!

@fullofcaffeine
Copy link
Member

I'm still confused as to why #35802 ends up causing the call to return null on its first call, while before it, it'd return the data. It might just be an (expected) side-effect from the changes that I still don't understand.

@Mamaduka What you mention in your comment here makes sense. I wonder if the it's somehow related to what @ockham mentioned here, though (TL;DR: for some reason #35802 caused that endpoint not to be preloaded anymore?).

Don't feel rushed to reply to this, I just wanted to try to clarify a bit further but I understand that it's not as critical as we originally thought (or might not be an issue at all, atm).

@Mamaduka
Copy link
Member

@fullofcaffeine, the "Navigation Panel" was restored in 11.9.1 (#36516) and now running the following selector returns values from the state:

wp.data.select( 'core' ).getEntityRecords( 'postType', 'wp_template' );

I've not done a git bisect for this issue myself, but I don't think #35802 changed anything based on this.

@fullofcaffeine
Copy link
Member

@Mamaduka Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

No branches or pull requests

6 participants