Add mutations data and helper functions to useEntityRecord #39595
Conversation
adamziel
requested review from
nerrad,
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
adamziel
added
[Type] Code Quality
I’m on board with the proposal. I totally agree with the reasoning shared. In the diff of this PR, I see that it actually should be Both approaches have pros and cons. When using two separate hooks, it might be easier to distinguish what’s the difference between I guess we would have to try some refactoring in Gutenberg with the current API to gather more information before drawing conclusions. Anyway, it’s great to see constant, interesting explorations in this area ❤️ |
adamziel
force-pushed
the
hooks/entity-mutations-use-entity-record
branch
from
06f1c6d
to
8501dc7
Compare
adamziel
changed the title
adamziel
force-pushed
the
hooks/entity-mutations-use-entity-record
branch
from
8501dc7
to
e7276d5
Compare
|
Size Change: +940 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
@gziolo interestingly, most usages of To me, that's a signal this PR is not as relevant in the Gutenberg repo at the moment. It could be still useful for extenders. What do you think? edit: here's a few cross-searches I've ran to find files that match both e.g. |
I guess for Gutenberg in the majority of cases we call Now, the question would be: how do we get feedback from extenders about the API proposed? |
Great question! I wonder if @annezazu may have some insights here |
|
For reference, here's how the Without this PR merged: const { page, lastError, isSaving, hasEdits } = useSelect(
( select ) => ( {
page: select( coreDataStore ).getEditedEntityRecord( 'postType', 'page', pageId ),
lastError: select( coreDataStore ).getLastEntitySaveError( 'postType', 'page', pageId ),
isSaving: select( coreDataStore ).isSavingEntityRecord( 'postType', 'page', pageId ),
hasEdits: select( coreDataStore ).hasEditsForEntityRecord( 'postType', 'page', pageId ),
} ),
[ pageId ]
);
const { saveEditedEntityRecord, editEntityRecord } = useDispatch( coreDataStore );
const handleSave = async () => {
const savedRecord = await saveEditedEntityRecord( 'postType', 'page', pageId );
if ( savedRecord ) {
onSaveFinished();
}
};
const handleChange = ( title ) => editEntityRecord( 'postType', 'page', page.id, { title } );With this PR merged: const { record, edit, save, lastError, isSaving, hasEdits } = useEntityRecord( 'postType', 'page', pageId );
const handleSave = async () => {
const savedRecord = await save();
if ( savedRecord ) {
onSaveFinished();
}
};
const handleChange = ( title ) => edit( { title } );(note: lastError is missing from this PR at the moment) |
|
What kind of feedback are you looking to get? :D Here are some ideas:
Happy to help with any of the above/help coordinate. |
|
@adamziel, the example from documentation you shared: const { record, edit, save, lastError, isSaving, hasEdits } = useEntityRecord( 'postType', 'page', pageId );
const handleSave = async () => {
const savedRecord = await save();
if ( savedRecord ) {
onSaveFinished();
}
};
const handleChange = ( title ) => edit( { title } );This looks great! We need more existing code like that to confirm we didn't miss anything. I agree we could use WP Slack and/or Twitter to find some plugin authors who might benefit from the API changes introduced. Great idea @annezazu!
That's also an option because @wordpress/core-data has already the stable version of |
|
Thank you @annezazu and @gziolo ! I've run the following searches:
Here's what I found: Plugins that could benefit from this PR:
Plugins that wouldn't benefit from this PR today:
The code samples I've analyzed follow the same pattern as I described in my last comment, e.g. Now that we've established there's at least a few plugins that would benefit here, a Make Core post sounds like a reasonable next step. As a side note, I wonder how much of a chicken&egg this is, e.g. people aren't using core-data utilities because they are not developer friendly and well documented. |
That's a good point. It might be also a few other things like using
Let's do that step and see whether it helps with making the final call. At this point, my only concern is backward compatibility if we change our mind about the shape of the API a few months later. If that would be internal to the Gutenberg plugin I would be more than happy to ship it now. |
adamziel
force-pushed
the
hooks/entity-mutations-use-entity-record
branch
from
713dbf9
to
b019462
Compare
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
|
The only important feedback I have is included in #39595 (comment). I was wondering whether we could optimize the usage of |
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
…:WordPress/gutenberg into hooks/entity-mutations-use-entity-record
|
|
||
| function PageRenameForm( { id } ) { | ||
| const page = useEntityRecord( 'postType', 'page', id ); | ||
| const [ title, setTitle ] = useState( () => page.record.title.rendered ); |
There was a problem hiding this comment.
One thing that still isn't clear to me. Why do we need useState here rather than using const title = page.editedRecord.title and const setTitle = ( value ) => page.edit( { title: value } )?
There was a problem hiding this comment.
const setTitle = ( value ) => page.edit( { title: value } ) would make undo work character by character :( I considered documenting that behavior in this code snippet and ended up leaving it out. There's already a lot of concepts in there.
There was a problem hiding this comment.
I'd love to go ahead and merge this PR since there are no conflicts and the checks are finally green :D we can tweak the docs in a follow-up if needed.
There was a problem hiding this comment.
I approved the PR. I'm wondering how the undo/redo works with setAttributes from the block edit API. I don't see there an issue with storing all atomic operations when using onChange with TextControl.
There was a problem hiding this comment.
I figured out why it works in a special way for block attributes. There is this util that checks whether the changes get applied to the same block's attribute:
gutenberg/packages/block-editor/src/store/reducer.js
Lines 154 to 172 in e2b293d
It's used by the higher-order reducer that ensures that only a single entry gets added to the under/redo state for subsequent changes to the same attribute:
gutenberg/packages/block-editor/src/store/reducer.js
Lines 463 to 517 in e2b293d
Maybe we should figure out how to mirror the same behavior for entities to unify the experience and bring more value to using editedRecord and the edit method from this hook.
There was a problem hiding this comment.
Nice work extending the API for useEntityRecord. I find it as a very good improvement over what we had before.
It feels like the underlying concepts like records vs editedRecords and edits vs save still need some improved documentation as the included code example doesn't fully present what are the best practices for using them.
What?
This PR proposes merging entity mutations utilities into the
useEntityRecordhook. This is an alternative to #38135 and was earlier discussed in #39258.The rationale is that manipulating entity records in core-data is quite difficult these days. Building a simple form currently requires a snippet like this:
I'd like it to look more like this:
Different angles
Re-throwing errors is different than having a convenience wrapper for actions like
editEntityRecord,saveEditedEntityRecord,saveEntityRecordand thegetEditedEntityRecordselector. Perhaps that's where a hook would actually shine:However, given the "throwing" actions, accessing
errorandisCreating/isSaving/isRemovingwould not be needed as often, which leaves us with just:Which doesn't do much for creating and removing records so we could also ditch these hooks and settle only for the
useEntityRecordhook:Dev note
The Make Core post can be easily repurposed as a dev note.
cc @gziolo