`( 'core/edit-site' ).getEditedPostId()` returns a number instead of a string

gigitux commented 1 year ago

Description

https://github.com/WordPress/gutenberg/pull/52338 (shipped with Gutenberg 16.3.0) converts the postId into a number and, after that sets in the redux store the number (source code). According to the JSDoc getEditedPostId should return a string | undefined type. (source code).

In WooCommerce blocks, we rely on this return type (source code), so the code isn't ready to work with a number type. This introduces a regression in WooCommerce Blocks plugin. I suspect that other plugins could have the same issue.

Step-by-step reproduction instructions

Open the Site Editor.
Click on the navigation.
Click on the pencil (to edit button).
Open the dev tools and run: window.wp.data.select( 'core/edit-site' ).getEditedPostId()
The return type is number instead of string.

Screenshots, screen recording, code snippet

WordPress 6.3	WordPress 6.3 with Gutenberg 16.3.0 installed

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

nerrad commented 1 year ago

For additional context, the templates entity has id as a string in the REST response, whereas other post entities return id as a number in the REST response (according to schemas). I haven't dug in to see how the entity data store treats the response but I seem to remember it just storing what is provided by the REST response without changing the type.

bph commented 1 year ago

Regarding 6.3 release cycle.: The typeof is still "string' in WordPress 6.3 RC 2. It doesn't seem that the PR causing the type change made it into the WordPress release.

Marc-pi commented 1 year ago

3 issues related yet linked to this regression "The error reproduces consistently with WooCommerce v7.9.0 and v7.8.2 alongside Gutenberg v16.3.0. Using Gutenberg 16.2.1 does not result in the same error - the navigation editor loads fine using that version."

scruffian commented 1 year ago

I think the postID should be a number, not a string, so we should update the docs to reflect that.

nerrad commented 1 year ago

I think the postID should be a number, not a string, so we should update the docs to reflect that.

That may not necessarily be true for templates though (which are currently strings):

console.log( wp.data.select( wp.editSite.store ).getEditedPostId() );
// in the context of a site template returns something like:
// 'twentytwentythree//home'

It's a bit unfortunate that postID is overloaded to be multiple potential types in this environment, but to be accurate the type would still need to be string | number | undefined.

draganescu commented 1 year ago

We'll see how to approach this best. Given that the PR only casts to number if the value looks like a number, it doesn't look like something we should rush to revert or unfix - particularly since the PR did not land in the core releases.

The problem we're trying to fix is caching mismatches because of the inconsistency in the type of post id. As I've argued in #52120 I think casting the type to fix caching is a bad solution, and that we should instead cache API responses with, as @jsnajdr suggested, some serialized form of the request, not with one particular multitype prop.

If we approach caching correctly #52338 should be reverted and #52120 be closed. On the other hand @jsnajdr and Felix approved #52120 which may cause the same problem for Woo, right? So maybe that is worth testing and getting some feedback in place for #52120.

As far as the problem itself - post ID being string | int - just saying it sounds weird. If we had a post key or something similar then it would have made more sense, but generally ID is unexpectedly a string. Add to that that sometimes the ID is a Number but it comes as a string from the API. So, we should try to add some consistency there as much as possible - but my personal preference is to solve this in the API - the client is too late to enforce opinions on the data it gets, I believe. So if the ID looks like a Number just send a number, don't just default to a string.

Let's see what @getdave has to add in terms of how to continue best.

getdave commented 1 year ago

I agree with @draganescu that the issue here really is that for templates on the REST API we (historically) decided to make it's ID a string-based slug. I believe that we should have instead used another field altered the way we consume the endpoint to handle looking up templates by slug. However, that's now in the past and I don't think we can alter the REST response of something that is in Core already.

I'm looking at the PR referenced here and I didn't actually change the selector. But, what I did do was set data in the store and the selector doesn't enforce the return type to match what it specifies. That's the source of the bug.

In WooCommerce blocks, we rely on this return type (source code), so the code isn't ready to work with a number type.

This seems entirely reasonable. The selector should return the correct type or we should amend the selector. However, it's extremely confusing that an ID can be a number or a string slug. Again, this is a historical problem.

...the client is too late to enforce opinions on the data it gets, I believe. So if the ID looks like a Number just send a number, don't just default to a string.

If we can do this then it seems to make sense. Cast the type on the REST API. However, if we look at the Posts endpoint as an example the type of ID is already integer so I assume the issue with storing IDs is actually somewhere else.

Next Steps

If it helps I agree we could revert https://github.com/WordPress/gutenberg/pull/52338. It will have no adverse effects other than additional network requests.
https://github.com/WordPress/gutenberg/pull/52120 is a more involved solution but one which we're still exploring to find the right approach. Don't think we need to close it at this stage.
Investigate the response from the REST API and determine if/when it's cast to a number internally. My research in https://github.com/WordPress/gutenberg/issues/52106 should help.

getdave commented 1 year ago

Here is the revert PR for review https://github.com/WordPress/gutenberg/pull/53419

merijnponzo commented 1 year ago

It would be very helpfull if getEditedPostId() would return the same id of the wp_template_part as within the wp_posts table is used.

I would also expect that getEditedPostContext() would return the edited content of my template part but it's returning undefined.

document.addEventListener("DOMContentLoaded", function () {
  if (typeof wp !== "undefined") {
    if (wp.data !== undefined) {
      const { dispatch } = wp.data;
      wp.data.subscribe(() => {
        // is within site editor
        if ( wp.data.select( 'core/edit-site' ) ) {
          const templateType = wp.data.select( 'core/edit-site' ).getEditedPostType();
          const templateId = wp.data.select( 'core/edit-site' ).getEditedPostId();
          const templateContent = wp.data.select( 'core/edit-site' ).getEditedPostContext();
          console.log(wp.data.select( 'core/edit-site' ), templateType, templateId, templateContent)
        }

returns {object}, wp_template_part, ponzotheme//header, undefined

WordPress / gutenberg