Fix resolver resolution status key types mismatch causing unwanted resolver calls for identical state data #52120
Conversation
|
Size Change: +144 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
This comment was marked as resolved.
This comment was marked as resolved.
842b6f3
to
8462ae4
Compare
|
Can I ask why do we want to consider "1" and 1 as equivalent in the first place? Is it certain that we want to do this or is this a bug elsewhere (where we lose the actual type of the identifier) |
We only want to do this in the case where {
title: "My Post",
id: 123,
type: 'some_post_type'
}...these two calls are asking for the same data: // String as Post ID as record key
getEntityRecord('postType', 'some_post_type', '123');
// Number as Post ID as record key
getEntityRecord('postType', 'some_post_type', 123);If we don't do this then the cache will think it's empty and the resolver will be triggered even though it's already been resolved for the given
No I don't believe it's a bug "with type "loss". The type is passed to the selector/resolver when it is called incorrectly with a The caller is wrong and the resolver is just doing what it should do. We can protect the caller from this bug. It is being "clever" so there's an argument to just leave it "as is", but that results in additional network requests as I hope I've explained in the Issue and this PR. Alternatives consideredOne other option might be to introduce a new selector/resolver specifically for fetching records by Post ID. Something like Reasons I don't favour that:
More ContextThe issue that for // String as Post ID as record key
getEntityRecord('postType', 'some_post_type', '123');
// Number as Post ID as record key
getEntityRecord('postType', 'some_post_type', 123);There are no guards in place and nor can we add any because there are cases when // Slug as recordKey
getEntityRecords('postType', 'wp_template_part', 'twentytwentythree//header');This flexibility is great except that it allows those who are intending to pass a post ID as Now we could say "everyone should know to pass We could of course patch the individual instances within the Gutenberg repo where As far as I can see there isn't a cleaner solution to this problem but I'm open to discussion. |
I mean if third parties are doing it wrong, it's also their problem. I'm "ok" with this normalization but can't stop but wonder whether it's really necessary. Is there a lot of places where we use "strings" instead of "numbers" in Gutenberg? |
Yes, but unlike with many other WP APIs we don't alert to the user that they are "doing it wrong" because strictly they aren't. It's just our cache (which is masked from the caller) is absolutely strict about types. In this case we allow them to make this call and they are absolutely none-the-wiser to the performance impact unless they happen to be monitoring network calls. On a slower network having to retrieve a record from REST over the network instead of from state could be pretty impactful. Personally I spotted this when going from Navigation listing -> Single Navigation view in the Site View. Having to wait for the record to resolve again is a pretty poor experience and it's exacerbated by the fact that we've already preloaded the navigation menus query on the server.
Understood and I agree It's not a clear cut problem -> solution. I found a few instances in Core but admittedly it's not a lot, largely thanks to the fact that we tend to store block attributes with specific types. One area where we are open to bugs however, is when retrieving params from the URL. If, for example, I get 
All I need do is inadvertently pass that to I definitely see the argument that this isn't 100% critical, but it's a nice quality of life improvement and also guards against performance issues which definitely possible (and potentially even likely, depending on how we store data). Another alternative is that we trigger a console warning if we suspect a caller is passing the wrong argument type? Do we think it's worth me committing any more effort here or shall I focus elsewhere? My opinion is that it is worth it but I'm just one contributor 😄 |
It might be that calling selectors with "wrong" argument types is a very common mistake, it just goes undetected. Because the consequences are not that bad, it's just that the potential resolver is called twice. If your state is stored as an object, then It seems to me that mixing up |
|
I'll proceed then 🙇♂️ |
|
I'm now stuck with TypeScript in allowing the |
…matching resolver
As per code review
fb67d28
to
5d2c7b3
Compare
What?
Stops unwanted resolver calls due to resolver cache misses.
Specifically fixes
getEntityRecordin@wordpress/core-datato avoid cache miss caused by type mismatch on therecordKeyargument.Closes #52106
Why?
The following selector calls are not considered equivalent by the caching mechanism in
@wordpress/data(see #52106 for detailed explanation):This means that even if the record exists in state, the relevant resolver is not marked as resolved and thus the resolver runs again thereby triggering a network request.
This is very likely to occur because in Gutenberg
IDs can be strings which means a lot of (potential) cache misses and unnecessary network calls and loading delays.A specific example is the
useNavigatorhooks where the resultingparamsobject will contain params as strings even if they are numeric. So if there's a param ofpostIdthen it will be a string. It's very easy to accidentally pass that togetEntityRecord.How?
Adds a new optional property to the
resolverselector which allows the implementor to define a method which normalizes theresolverargspassed to the selector into to a standard form. This standardised form is then used in cache set/get.This PR implements such a method for the
getEntityRecordresolverselector to stabize the type of therecordKeyas a number rather than a string when therecordKeyargument looks like a numberic ID (which is likely to be the ID of a Post record).Testing Instructions
Escto also open theConsoletab below it.wp.data.select('core').getEntityRecords('postType', 'wp_navigation'). You should seenulland then running it again you should see results. This has populated thecore/datastate with entities of that type.ids of those records.getEntityRecordsresolves thegetEntityRecordselectors.wp.data.select('core').getEntityRecord('postType', 'wp_navigation','{YOUR_RECORD_ID}')GETrequest (ignoreOPTIONS) towp/v2/navigation/{RECORD_ID}.recordKeyargument aNumber....you can also test this in the Site Editor by:
Navigationin Browse Mode sidebar.GETnetwork request was dispatched.Testing Instructions for Keyboard
Screenshots or screencast