Revisions: add new selectors to fetch entity revisions #54046
Conversation
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
|
Size Change: +559 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing nicely for me so far. While it's a bit awkward passing values to the query object that won't really be used in the query, but instead will be used as part of the API url, it feels to me like a natural place for the code calling getEntityRecords to place it, and if it's something that's particular to the revisions kind then maybe it'd kinda work? Also I quite like how you're taking the value and then discarding it before passing the rest of the query on via getQueryArgs.
Thanks for looking at this idea 🙇🏻 I think my inexperience with this package shows 😄 But it seems to handle things, I guess we just need to work out a better interface. I think perhaps a wrapper for For better or worse, the following also crossed my mind: // A wrapper for `getEntityRevisions`
getEntityRevisionsRecords( 'revisions', `${ postType }`, `${ parentId }`, { ...query } );
// An extra options arg to house stuff.
getEntityRecords( 'revisions', `${ postType }`, { ...query }, {
parent: parentId,
} );
|
|
I've been tinkering with different ideas today. Jotting down some very random findings and questions. At this stage Gutenberg only displays global styles revisions, but my instinct is to build something that other entity types (post, page, templates) can hook into. The first question would therefore be: Where to place revisions in state? Loosely, I think it should be filed under entity name (post type), e.g., {
revisions: {
parentId: {
items: { revisionId: { ...revisionData } },
...
}
}
}This is what I was trying in https://github.com/WordPress/gutenberg/compare/try/core-data-entity-revisions-under-name Or could they be a separate top-level item {
revisions: {
[ name ]: {
parentId: {
items: { revisionId: { ...revisionData } },
...
}
}
}
}Either way, it would be great to use existing query reducers to track what's already in state. I haven't very far with that yet, but I suppose they'll need to be modified, along with a bunch of other code that expect specific properties. By query, I mean modifying REST query params such as I was thinking we could invalidate the query cache if the revision count has changed. Furthermore, give the above we could, later, deprecate Maybe I'm overthinking it. It's a lot of work and might introduce bugs at first, so another approach would be to just implement global styles revisions (all new code and top-level state item) and custom query handling in state. Or just concentrate on global styles revisions and not worry about caching state according to query (means more API calls) for now. @youknowriad I know you've got a lot on, but when you get a spare moment it would be great to hear your thoughts. I'm wary of touching core data and breaking stuff, but don't want to repeat too much either. 🙏🏻 |
|
This is a very quick though passing around here.
|
I appreciate the feedback - it's very valuable, thank you. I'll have another poke around.
The main concern I had was not to reproduce logic surrounding query caching, but maybe it's best to do it longhand and look for consolidation opportunities later. It would at least suit me if I were to carry on: me having not much experience with the entity types and models in the plugin 😄 |
|
I think it would be valuable to have other opinions here as well. cc @WordPress/gutenberg-core @jsnajdr I agree that query caching and handling is a bit of a monster and that reusing it helps a lot. Maybe there are ways to reuse these sub-reducers/sub-selectors without allowing fetching revisions as entity records. Anyway, I'm very open to more discussion and opinions here :) |
|
To me post revisions seem to be a full-featured REST entity on its own. For a given post type and post ID, there is a collection of revisions, and also individual revisions. Each of them having their own REST resource URL. The relationship between a post and its revisions is similar to the relationship between a post and its attachments (media) or its comments. They are all linked to in the
Yes this is one thing that's unusual about revisions. A revision has a perfectly unique and canonical revision id -- after all, it's just a post like any other, with post type The endpoint doesn't even care if the revision 2's parent is really the post 1. If will happily return the same revision from URL like: It merely checks that |
|
Could making the revisions(version-history) |
|
If we were to consider revisions an entity.
I think revisions are "sub entities" and not top level entities and in that sense, our framework doesn't support sub entities yet. |
|
I'm really grateful for the discussion. I was swimming in the dark on this one 😄
I'd guess that it would return a set of revisions according to the default collection params? So max items 100, ordered by date descending.
I was smitten with an earlier idea of yours: to create a dedicate selector. This could remove the need for argument gymnastics.
Would that impact response size for posts with lots of revisions? Or could we embed a subset? 🤔
Interesting. As in Where do folks think it would make sense to build this into the existing state? At first I was imagining something like this:
I'm guessing we'd fetch the entity record first and then grab the revision URL? E.g, I suppose I'm wondering if we need to care about the revisions URL in the same way we do about top-level entity |
I think that's not important at all, it's implementation details, so whatever feels easy. For me, the thing to get right is the external API (selectors and actions). |
This collection doesn't exist, at least there is no REST API to fetch it.
Yes, this is IMO the only thing where post revisions differ from other entities. And the tuple wouldn't be the id ( getEntityRecords( 'postType', 'post:1:revisions' ); // collection
getEntityRecord( 'postType', 'post:1:revisions', 2 ); // itemThe first two arguments, https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/resolvers.js it needs
Not really because things that are embeddable are always already independent entities. Embbeding But here we don't even have an agreement that revisions are a real entity 🙂
If we started supporting sub-entities, how would the API look like? Some nested call like this? getSubEntityRecords( getEntity( 'postType', 'post', 1 ), 'revisions' );But that's not really different at all from a compound entity name: getEntityRecords( 'postType', 'post:1:revisions' );
I disagree here. Revisions are an entity on its own, simply because they have their own REST endpoint. That's the necessary and sufficient condition. Comments and attachments are also "properties or another record": the REST response for a post contains links to them in the Revisions just have a weird |
It doesn't matter if it doesn't exist, the API in core-data exists If
While this makes more sense in terms of API calls . I don't think we ever planned for "name" to be this dynamic: aka create an entity per post ID. Also it doesn't make sense because the config for
I don't think we need to necessarily support entities in a generic way from the start. I think we should start by just supporting "revisions" for entities Potentially we could support sub entities more generically later, if there's a need to fetch more sub entities outside of revisions. but there's no need for a generic selector. |
If
We never planned for this, but suprisingly, it could work very well 🙂 The configs for both posts are only almost the same: the
This is the same thing as So, I don't mind starting with |
|
Once again, thanks for sharing your knowledge. 🍺
This could be a silly question: I was wondering, could revisions be its own "kind"? That way it could run through the post types that support revisions. I tried that using a tuple-like key:
Here is 
A far few unnecessary properties that could be pruned from the reducer, e.g., saving/edits/deleting To do this I created a callback |
To answer this we need to know what The only place where When deciding whether The answer will follow from how the code for revisions looks like. Does it call const config = select.getEntitiesConfig( 'postTypeRevisions' );
const revisionsUrl = urlFromConfig( config );or const config = select.getEntitiesConfig( 'postType' );
const revisionsUrl = urlFromConfig( config ) + '/revisions';? Both is possible, although I personally would intuitively prefer the second option: load entity configs for the |
ramonjd
force-pushed
the
try/core-data-entity-revisions
branch
from
b9d768c to
0edea5d
Compare
Interesting! Thanks again for helping on the thinking. I had some time today so I hacked something into this POC PR just to test it out (and learn more about the innards of core-data) |
ramonjd
force-pushed
the
try/core-data-entity-revisions
branch
3 times, most recently
from
6f0490d to
65e41f2
Compare
|
Flaky tests detected in 68a1581. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6739002166
|
ramonjd
force-pushed
the
try/core-data-entity-revisions
branch
3 times, most recently
from
21fcb95 to
37e23db
Compare
I'll do this as a follow up. |
There was a problem hiding this comment.
Just gave this another test, and I'm really liking the shape of the API via the getRevisions and getRevision methods 👍
One thing I noticed running locally is that when calling the getRevisions selector with different params, it'll immediately return the results of the previous call while it's resolving the request for the next call. I wasn't sure if this is just the way that these kinds of selectors are meant to work, or if there's a potential bug there. Here's an example:
In the above screengrab:
- 1st call (per page of 2): returns
nullas no data has been fetched yet (I think this is expected) - 2nd call with same params: returns the data previously fetched (array of size 2)
- 3rd call (per page of 3): returns the data previously fetched (array of size 2) — this is the response that seemed unexpected to me. What should it return immediately? Perhaps
nullbecause it's meant to be resolving the request? - 4th call (per page of 3): now returns the data fetched for 3 items
- 5th call (same params): still returns expected results
Once data has been fetched via the REST API, all the responses to calling the selectors are as expected. It's just that first call with new params where it looks like it might be returning stale data immediately before it then does the call — in that case, I would have expected it to return null or undefined as in #54046 (comment) instead of the response from the previous call (that may have had different params).
Apologies if this is a red herring or if it's expected behaviour!
|
@andrewserong Thanks for testing again. I had the same question. See comment. TL;DR: it's expected 😄 I've updated the test description to use 
|
My original comment was about the |
|
The difference for per_page is confusing to me too. I don't understand why we'd show page 2 results and then switch to page 3 results. If the returned result is the accumulation of all the pages, sure, they should be considered the same query but if we're actually returning just the page 3 in the end, I think that's a bug. But it's independent from this PR, maybe let's track it for now. |
|
Thanks for the explanation, folks! @andrewserong sorry, your comment flew over my head 🤦🏻 I get it now 😄 I can write up an issue for this. |
|
The reason why I thought it was all fine is because I've been resolving each call via So there are 2 x API calls as I'd expect, and the results also match what I'd expected, that is, the correct revision ids in the collection. 2023-11-21.07.45.01.mp4
Maybe we need to inform the master selector from the queried data selector somehow that the query key doesn't yet exist. My guess is that it's fetching it from the cache despite that. 🤷🏻 Not sure. I can take a quick looksy. Thanks again all for helping with this PR. It's going to enable a lot of future work. 🍺 🍺 🍺 🍺 🍺 🍺 🍺 🍺 🍺 |
ramonjd
added
the
Needs Dev Note
|
Note to self: this feature probably deserves a dev note post on https://make.wordpress.org/ |
There was a problem hiding this comment.
But it works this way consistently across all core queries.
Thanks for confirming and for the extra discussion, folks! +1 this PR sounds good to merge, and we can look into the issue of potentially stale data being returned separately 👍
Nice work getting this into good shape @ramonjd! ✨
Issue: Not sure if this is a viable fix, but tried it out: |
Dev noteMilestones: Gutenberg 17.2 Two new selectors have to been introduced to Block Editor's Core Data API to fetch revisions and single revisions for post types that support revisions, for example, posts and pages, templates and global styles. getRevisions( kind, name, recordKey, query ) – fetches a post’s revisions where recordKey is the id of the parent post.
getRevision(kind, name, recordKey, revisionKey, query) – fetches a single post revision, where revisionKey is the id of the individual revision.The functions use similar arguments to exisiting core data entity functions such as Example usage: // Returns a collection of revisions.
// `parentGlobalStylesId` is the id (int) of the parent post.
wp.data.select( 'core' ).getRevisions( 'root', 'globalStyles', parentGlobalStylesId, { per_page: -1 } );
// Paginated results.
// `parentId` is the id (int) of the parent post.
wp.data.select( 'core' ).getRevisions( 'postType', 'post', parentId, { per_page: 3, page: 2 } )
// Get a single revision object.
// `parentId` is the id (int) of the parent post.
// `revisionId` is the id (int) of the individual revision post.
wp.data.select( 'core' ).getRevision( 'postType', 'post', parentId, revisionId );
// Get a single revision with only the id, parent and date fields.
// `parentId` is the id (int) of the parent post.
// `revisionId` is the id (int) of the individual revision post.
wp.data.select( 'core' ).getRevision( 'postType', 'post', parentId, revisionId, { _fields: 'id,parent,date' } );
import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
function MyComponent() {
const pageRevisions = useSelect(
( select ) =>
select( coreStore ).getRevisions( 'postType', 'page', pageId ),
[ pageId ]
);
// Do something with pageRevisions...
}In the background, these selectors’ corresponding resolvers call revisions REST API endpoints. |
vcanales
added
the
Backport to Gutenberg RC
madhusudhand
removed
the
Backport to Gutenberg RC
ramonjd
added
has dev note
Tracking:
What?
This PR fulfils the dream of bringing post type revisions to the Block Editor via the Core Data API. Get excited.
It adds two new functions to the global API:
getRevisions()getRevision()Both have slightly similar function signatures to
getEntityRecordsandgetEntityRecordrespectively. E.g.,Also, the same query params can be passed:
Why?
To get post revisions and unlock a treasure chest of UI explorations into how to display, compare and restore revisions in the Block Editor.
How
Entities receive the following new config entries:
getRevisionsUrl( parentId, revisionId ): to return the GET request URL to fetch a post's revisions or a single revision.revisionURLParams: similar tobaseURLParamsto provide the default params for the revisions request.supports: { revisions: Boolean }to indicate that an entity supports revisions. This information can also be fetched from/wp/v2/types?context=edit, but I suspect the response size is larger than what's received in the view context and causes the performance tests to fail. (see investigation below)Usage and testing instructions
To test, add a post or page with some revisions. Also add a few revisions to global styles.
You can use the Core Data API in your browser's console.
Test that revisions are removed when their parent is removed:
To see
getRevisionsin action in the Site Editor, copy the patch below and apply it to this branch (pbpaste | git apply)Then head to the Site Editor and open the revisions panel. Add some changes to Global Styles to make sure the revisions list updates as expected.
Follow up tasks/TODO
Tracked here:
Related