Use Core Data for Nav fallbacks and side load resulting entity into state #50032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
changed the title
getdave
commented
Apr 24, 2023
getdave
requested review from
spacedmonkey,
tellthemachines,
nerrad,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +110 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6b402d199a593b8ad6e5e9f137dcca997137db89. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4786394990
|
getdave
commented
Apr 24, 2023
getdave
commented
Apr 24, 2023
getdave
commented
Apr 24, 2023
getdave
commented
Apr 25, 2023
youknowriad
reviewed
Apr 25, 2023
getdave
force-pushed
the
add/core-data-selector-nav-fallbacks
branch
from
6b402d1
to
fa1a898
Compare
draganescu
reviewed
Apr 27, 2023
| @@ -482,6 +494,18 @@ _Returns_ | |||
|
|
|||
| - `any`: The entity record's save error. | |||
|
|
|||
| ### getNavigationFallbackId | |||
There was a problem hiding this comment.
My main gripe with this is that all methods are generic but this one is specific ... I wonder if there is any way around this but continuing to do exactly what this PR does. @youknowriad should we be able to add selectors and other core data stuff at the block level somehow but which work with / hook into the entity flow seamlessly? Is that even possible?
There was a problem hiding this comment.
I think it's fine that this is a specific selector, we have other ones like it: __experimentalGetCurrentThemeBaseGlobalStyles, __experimentalGetTemplateForLink, __experimentalGetCurrentGlobalStylesId to name a few.
I don't think we can conceptualize all the selectors from "core-data" (aka wordpress API) as generic entities selectors.
I think if you're still not satisfied with the API itself and you think you might want to change it a bit, you can make it private to start with.
There was a problem hiding this comment.
Yes @draganescu I also had that thought so I understand where you are coming from.
However, I also noted the selectors Riad has outlined above and decided it would be ok.
Happy to make private if you'd prefer for now and it would make it easier to land this selector?
There was a problem hiding this comment.
Chatting with @draganescu away from Github he feels that we should not be using Core Data for this operation. Rather we should limit the fetch to the block code whilst continuing to side load the resulting embedded entity into Core Data state.
I have a PoC for this in #50126.
As things stand the sideloaded entity does get added to Core Data state, but that doesn't seem to stop the network request triggered by getEntityRecord() being called elsewhere within the block code once the id is available. I don't know why that is...
@youknowriad Curious to see what you think of this approach vs the one you've approved in this PR.
There was a problem hiding this comment.
Chatting with @draganescu away from Github he feels that we should not be using Core Data for this operation.
Why?
As things stand the sideloaded entity does get added to Core Data state, but that doesn't seem to stop the network request triggered by getEntityRecord() being called elsewhere within the block code once the id is available. I don't know why that is...
The important thing is that the selector gives you the result right away. If it's important for you to also avoid triggering the request, you can mark the "getEntityRecord" resolver as "resolved" (I think you need to dispatch "FINISH_RESOLUTIONS" or something)
There was a problem hiding this comment.
I think the block should be able to tell core data "hey I got (somehow) this data which is the entity i want to work with" and core data will adopt that entity (which was not retrieved via a selector/resolver) and then handle edits, undo, updates
I don't think this is an Issue. What you describe is already working today. It's implemented in this PR and the spin off PoC I created. To be clear here is the code:
if ( record ) {
// This is dispatched to Core Data
dispatch.receiveEntityRecords(
'postType',
'wp_navigation',
record
);
}The selector is block specific and the endpoint is also block specific today (nothing else uses that). It's an utility endpoint created for the navigation block to avoid doing all that on the client. I
This is the point of contention. I don't think it's a major problem to have this selector.
@draganescu In the interests of moving forward would you be open to making it a private selector and then make it public only if/when we confirm that other areas will also require it (for example the Navigation section in the Site View sidebar). We get many benefits by using Core Data here and there are also already many examples of the same pattern in use within Core Data.
There was a problem hiding this comment.
Absolutely @getdave - I expressly did not block this with my opinion :) - it's an optional nice to have considering core data selectors already do this for other blocks.
There was a problem hiding this comment.
Ok private it is. We'll go from there. Thanks both.
There was a problem hiding this comment.
@draganescu looks like you went ahead and merged. Do you still want me to follow up with a private selector?
|
@youknowriad I am still sometimes seeing the Do you think I ought to call... dispatch.finishResolution( 'getEntityRecord', [
'postType',
'wp_navigation',
fallback?.id,
] );...here to mark the query as resolved? |
|
Yes, I think that if you want to avoid the request from triggering entirely, we have to call the finish resolution API. |
This was referenced May 4, 2023
ellatrix
reviewed
Apr 1, 2024
| */ | ||
|
|
||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| setRef( navigationFallbackId ); |
There was a problem hiding this comment.
Is it normal that this causes a navigation block's attributes to be updated just by mounting it in the editor? That shouldn't be happening normally.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds Core Data selector to retrieve Navigation fallback and sideload embedded entity into state.
Closes #50002
Why?
Currently on
trunkthenavigation-fallbacksendpoint is called directly from the block'seditcode usingapiFetch. This means that each instance of the Nav block trigger's its own request which can result in multiple network requests (especially when multiple blocks are in the editor).Moreover, once the request has resolved with an
ida separate request is then dispatched to retrieve the full Navigation post object for usage within the block. Having x2 network requests is wasteful and slow and results in longer load times for the block in fallback mode.This PR solves both issues.
How?
This PR adds a selector to Core Data which calls the endpoint to retrieve the fallback. This means that only a single request is ever dispatched.
On top of this the endpoint is called with the
_embedquery arg, which causes the endpoint to response with the full Navigation post object embedded within the response under theembeddedkey. This post object is then pushed into the Core Data state which means that the network request resulting from callinggetEntityRecord('postType', 'wp_navigation', %%ID%%)(elsewhere in the Nav block code) is not dispatched. Instead the selector immediately resolves with the data that is already is state.Note that by default on certain fields are marked as being available in the
embedcontext. Therefore we are filtering the endpoint schema to include the necessary fields.FAQ
Why not simply return the Post object from the REST endpoint and avoid all this sideloading?
Initially I was returning the full Navigation post object from the REST endpoint. However, this required the introduction of a endpoint schema that married with the full Post object.
I spoke to Core REST folks about this and they advised that as we'd (collectively) agreed not to extend the Posts Controller, the most suitable option was simply to return a reference to the Navigation Post (i.e. the
id) and then include aselflink within the response which could optionally be embedded.This resulted in a more manageable schema whilst also retaining the ability to access the full post object as part of the request if necessary.
This is why the selectors return the ID and not the full Post.
Testing Instructions
navigationnavigation-fallbacks- check the response to see that the full post object is included under theembedded.selfkey.GETrequest (there will still be anOPTIONSrequest) to fetch the Navigation by ID (e.g./wp/v2/navigation/{{SOME_ID}}.Testing Instructions for Keyboard
Screenshots or screencast