[core-data] throwOnError option for saveEntityRecord and deleteEntityRecord actions #39258
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
adamziel
changed the title
|
Size Change: +2 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
adamziel
requested review from
ajitbohra,
mkaz,
ryanwelcher,
draganescu,
talldan,
tellthemachines,
kevin940726,
getdave and
noisysocks
as code owners
adamziel
added
[Package] Core data
adamziel
changed the title
|
Looking at the resulting code, somehow the more verbose code seems more readable? It's like a story. |
|
You didn't used to be able to I like the idea, but I don't find the API to be very intuitive if I'm honest. That is, when I read the code, I don't instantly know what it will do. What do you think about a param? await saveEditedEntityRecord(
'postType',
template.type,
template.id,
{ throwOnError: true }
); |
gziolo
reviewed
Mar 11, 2022
|
I'm not familiar with the existing codebase that uses the flow that is discussed in this PR, but I must admit that the proposal from @noisysocks in #39258 (comment) sounds reasonable. This way we can reduce the boilerplate code and we still leave the choice to the developer how it should be handled. The challenge for public APIs is that you rather want to minimize the number of methods exposed to optimize for the most common usage and easier discovery. This goes in contrast to the consuming projects where a new method might help to make the code more readable.
Thank you for sharing the full context and your thoughts process. It really helps to understand how you arrived at the final proposal and more importantly leaves a great archive that we can use in the future when evolving all those APIs around entities. |
I like it much better @noisysocks, thank you! I just updated this PR accordingly.
My pleasure @gziolo ! I suspected that sharing just the final conclusion wouldn't be quite enough to explain the rationale here, I'm glad that was useful! I might start doing it more often when reasonable 🤔 |
adamziel
added
the
Needs Dev Note
adamziel
changed the title
adamziel
changed the title
gziolo
reviewed
Mar 11, 2022
gziolo
reviewed
Mar 11, 2022
We can also expose I guess for |
|
The e2e failures are unrelated to this PR, see https://core.trac.wordpress.org/ticket/55367 |
gziolo
reviewed
Mar 11, 2022
gziolo
reviewed
Mar 11, 2022
packages/core-data/README.md
Outdated
| @@ -70,6 +70,7 @@ _Parameters_ | |||
| - _query_ `?Object`: Special query parameters for the DELETE API call. | |||
| - _options_ `[Object]`: Delete options. | |||
| - _options.\_\_unstableFetch_ `[Function]`: Internal use only. Function to call instead of `apiFetch()`. Must return a promise. | |||
| - _options.throwOnError_ `[boolean]`: If false, this action suppresses all the exceptions. | |||
There was a problem hiding this comment.
Aside: I think @michalczaplinski raised it in other places that it's confusing that the same APIs are documented in two places - here and in docs/reference-guides/data/data-core.md.
It'd be a good follow-up.
There was a problem hiding this comment.
The fix is beyond this PR, but I fully agree
There was a problem hiding this comment.
Follow-up = new PR. So we are on the same page 😄
adamziel
force-pushed
the
propose/use-entity-record-crud
branch
from
291d425
to
1b92f4d
Compare
kevin940726
reviewed
Mar 14, 2022
packages/core-data/src/actions.js
Outdated
| @@ -273,6 +275,10 @@ export const deleteEntityRecord = ( | |||
| error, | |||
| } ); | |||
|
|
|||
| if ( error && throwOnError ) { | |||
There was a problem hiding this comment.
What if error is undefined or null or 0 or any falsy value? Even though it's an edge case, it should still throw such error 😅. I guess we'll need a flag like hasError to determine if it reaches the catch clause or not.
(Would be awesome if there's a test case for it too, but it's a nitpick.)
adamziel
force-pushed
the
propose/use-entity-record-crud
branch
from
e084468
to
761c82b
Compare
adamziel
force-pushed
the
propose/use-entity-record-crud
branch
2 times, most recently
from
b9104fb
to
ab24ce7
Compare
adamziel
force-pushed
the
propose/use-entity-record-crud
branch
from
ab24ce7
to
8561fc9
Compare
…ngside usage examples
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
adamziel
force-pushed
the
propose/use-entity-record-crud
branch
from
8561fc9
to
9f847af
Compare
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.
Description
This is a continuation of Entity records hooks.
An original idea was to make creating, editing, and deleting entity records easier by introducing a specialized convenience wrapper such as
useEntityMutation:The rationale – that the full flow is not easy to figure out, confusing at times, and it is easy to omit a part or two. For example, to edit a record, save it, and display a progress indicator and any error information, you need at least the following information and actions:
Diving deeper, we realized that it is hard to distinguish between creating and updating records when both use-cases are crammed within a single
useEntityMutationhook.In this PR, I explored three smaller hooks called
useEntityRecordCreate,useEntityRecordUpdate, anduseEntityRecordDelete. See the relevant commit.However, it quickly occurred to me that we only rely on all these selectors because we do not embrace the full power of promises. For example, the Gutenberg codebase commonly relies on the following pattern:
This looks a whole lot like what we do in PHP:
What if we used promises instead? Even better, what if we ditched the hooks and used the promises right in the store so that other stores could benefit from that as well? And so this is the proposal this PR makes:
It takes care of:
isSavingEntityRecordmay still be used when needed or more convenient.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
useEntityRecordEdithook:I'm interested in your thoughts about all that.
cc @gziolo @kevin940726 @getdave @noisysocks @scruffian @draganescu