Add isSaving and isDeleting to useEntityRecord, add props to existing…

[rmorse]

What?

This exposes isSaving and isDeleting on useEntityRecord.

Why?

useEntityRecord exposes nearly all actions and data releated to entity records, without having to go an use an additional useSelect to get anything else... except isSaving and isDeleting still require a seperate select call.

How?

I've added isSavingEntityRecord and isDeletingEntityRecord inside the useEntityRecord hook.

I did look at useQuerySelect but that seems to deal more with getting things and checking if a record has resolved.

The best place I figured would be directly inside the hook, in the existing useSelect.

Testing Instructions

You can now use useEntityRecord like this:

const {
	record,
	editedRecord,
	hasEdits,
	edit,
	save,
	isResolving,
	hasResolved,
	isSaving,
	isDeleting,
} = useEntityRecord( 'postType', 'page', postId );

Notice isSaving and isDeleting are now exposed.

Testing

I'm really not too confident with updating and writing the tests for this, but I've added these new props to the expected testing outputs, I've not written any additional tests to check whether isSaving and isDeleting actually work as expected.

I would be interested to learn how to do this though.

[Mamaduka]

I think we should also return defaults for these new values when queries are disabled.

if ( ! options.enabled ) {
  return {
	  editedRecord: EMPTY_OBJECT,
	  hasEdits: false,
	  edits: EMPTY_OBJECT,
  };
}

[rmorse]

After thinking about this some more, I think the last thing missing from useEntityRecord would be deleteEntityRecord as delete...?

Any suggestions on the best place to wire this in?

[gziolo]

I left a comment in the parent issue #56471 (comment). It would be great to find other use cases before committing to new API parts.

I’m also wondering now why @adamziel didn’t decide to include delete in there. One issue I see here is that after deleting a record, the entity would get into a strange state where nothing works as expected. However, the same story applies when you do it externally as you no longer can modify the record with the deleted id on the server 🤔

[adamziel]

@gziolo here’s my original rationale:

Different angles

Re-throwing errors is different than having a convenience wrapper for actions like editEntityRecord, saveEditedEntityRecord, saveEntityRecord and the getEditedEntityRecord selector. Perhaps that's where a hook would actually shine:

const { create, isCreating, error } = useEntityRecordCreate();
const { editedRecord, applyEdits, saveEdits, isSaving, error } = useEntityRecordEdit();
const { remove, isRemoving, error } = useEntityRecordRemove(); // it's "remove", because "delete" is a reserved keyword

However, given the "throwing" actions, accessing error and isCreating/isSaving/isRemoving would not be needed as often, which leaves us with just:

const { create } = useEntityRecordCreate();
const { editedRecord, applyEdits, saveEdits } = useEntityRecordEdit();
const { remove } = useEntityRecordRemove();

Which doesn't do much for creating and removing records, so we could also ditch these hooks and settle only for the useEntityRecordEdit hook:

const { editedRecord, applyEdits, saveEdits } = useEntityRecordEdit();

[rmorse]

If we have save() as part of useEntityRecord() (as it is today), should we also expose isSaving there, it seems they should go together in the same hook?

I like the idea of different hooks for different contexts (eg the edit context) but I personally don't see the harm in having them all under one umbrella hook (I realise I might not be viewing this with the big picture in mind), the useEntityRecord hook already exposes 80% of the things that you can do, including save() and edit(), is the suggestion that we move these out of that hook into something like useEntityRecordEdit() ?

[adamziel]

I don’t have a strong preference and will defer to @gziolo.

At the same time, just to nurture, isSaving etc. became a part of state because a few years ago you couldn’t easily use await. However, today, Gutenberg has this API:

try {
	const templatePart = await saveEntityRecord(
		'postType',
		'wp_template_part',
		{
			slug: cleanSlug,
			title,
			content: '',
			area,
		},
		{ throwOnError: true }
	);
} catch ( e ) {
	// Display an error notice
}

It solves the same problem of knowing „what state are we in”, only in a more ”JavaScripty” way. Would that work for your use-case? If not, maybe it’s indeed time to expose isSaving from that hook.

See also #39258

[rmorse]

Thanks for the info @adamziel - for me its just about making the various states of the entities use a similar API across the board.

It feels like useEntityRecord is a handy way to access an entity record/states/actions easily - however, I can do what I need to using seperate select()'s (and also your suggestion using await)

Saying that, the place you do the save is not always the same component you need the information on whether a save is in progress or not, my thinking is, as we're already recording the isSaving via the store, it seems handy/logical to expose it in a familiar format to the other properties of entities.

I guess I thought useEntityRecord was literally a hook to make DX of using entities easier - I think this would be a change in line with that thinking (but is that the core objective... I don't know 😕)

Some places in core would benefit from this change also such the navigation menu.

[adamziel]

You do make a good argument @rmorse! I wonder what's @gziolo take here.