-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Allow making context specific requests using the data module #32961
Allow making context specific requests using the data module #32961
Conversation
(Don't review this just yet, I have some local changes changing the approach entirely but I'm having trouble pushing them due to internet issues, I'll update here once I'm done) |
Size Change: +1.76 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
8b29ace
to
b284577
Compare
Ok I managed to push the changes, this is ready to review. |
Co-authored-by: Nik Tsekouras <[email protected]>
I'm adding this to the backport list to be able to use it to fix the permissions checks for the site blocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great addition and you did it amazingly fast after discussing it yesterday 🚀 !
Great work Riad!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The packages/core-data/src/reducer
reducer also handles the RECEIVE_ITEMS
action in the edits
part. Should it also check the action.context
field and ignore responses that don't have the edit
context?
The receiveEntityRecords
and removeItems
have an invalidateCache
flag. The getEntityRecords.shouldInvalidate
handler that looks at this flag should correctly and carefully choose the queries to invalidate: only the context-specific one for RECEIVE_ITEMS
, and all contexts for REMOVE_ITEMS
.
@@ -41,6 +41,7 @@ export function getQueryParts( query ) { | |||
perPage: 10, | |||
fields: null, | |||
include: null, | |||
context: 'default', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the context
query param is omitted, the default value is view
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we do enforce "edit" context in the API requests, even if the context is "view" by default for the server, that's why I opted on a new "default"/"empty" context in the frontend. Backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that calling getQueriedItems
without context
will issue an API request with ?context=edit
query param and will store the result under the state[ 'default' ]
key?
And that getQueriedItems( state, { context: 'edit' } )
will issue a request with the same ?context=edit
query param but this time will store the result as state[ 'edit' ]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that calling getQueriedItems without context will issue an API request with ?context=edit query param and will store the result under the state[ 'default' ] key?
In most entities yes, but entities can define another set of default REST API parameters.
I agree it's not ideal that edit
is different than default
in this cases, but solving it properly would have required a small change in the entities API (explicitly define a default context instead of relying on a more global baseURLParams). I think it's something we can try but I thought it was not important for v1 of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's not terribly important. I mostly wanted to confirm that this little flaw is really there, and that it's not just my misunderstanding.
I think creating a record in a one context, should invalidate queries in all contexts |
Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Nik Tsekouras <[email protected]>
closes #11643
We've been struggling with context specific data request for a long time in Gutenberg. This is due to the fact that the data module had always had a single representation of what an entity record could look like.
In this PR, I'm exploring allowing context specific requests. I'm doing so by separating the results from requests of different context, it's like considering these as different entities entirely.
Hopefully, this will allow us to get rid of all the adhoc
apiFetch
usage across the code base.