Skip to content
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

Handling mutation errors on per-store level #28

Closed
dkzlv opened this issue Feb 1, 2024 · 3 comments · Fixed by #36
Closed

Handling mutation errors on per-store level #28

dkzlv opened this issue Feb 1, 2024 · 3 comments · Fixed by #36
Labels
enhancement New feature or request

Comments

@dkzlv
Copy link
Member

dkzlv commented Feb 1, 2024

First mentioned in #27.

The proposed solutions are:

  • adding onError to createMutationStore interface. Maybe, same should be applied to createFetcherStore?
  • (not sure I like this direction) add a conditional to createMutationStore, if mutate should throw or not.
@dkzlv dkzlv added the enhancement New feature or request label Feb 1, 2024
@martinmckenna
Copy link
Contributor

Just wanted to pop in here to make a case for potentially adding an onError argument to mutate().

We have a use-case, where we're letting the user bulk-delete things like so:

const { mutate: deleteThing } = useStore($deleteThing);

const handleBulkDelete = (values) => {
  Promise.all(
    (selectedOptions).map(async (eachOptionToDelete) => {
      await deleteThing({
        thingID: eachOptionToDelete.id,
      });
    }),
  );
};

And since some can error, some can succeed, it would be good to handle errors on a per-request level, which means we would either need muatate to take it's own onError or have the option to have it throw

@dkzlv
Copy link
Member Author

dkzlv commented Feb 6, 2024

@martinmckenna Your particular code sample seems to be a strange usage of mutations 🤔 I'll set aside the fact that you perform N API operations instead of one (maybe, that's just a limitation of your backend, even though it's quite horrible, sometimes that's life). I particularly dislike the usage of the same mutation store in N parallel operations.
Even though it'll work, that's a bit misleading, because you know what those $deleteThing.get().loading/error will reflect? Only last mutation, and you'll never know how many of them are running, for example.
See the thing is that it could have been one mutation, right?

const $bulkDeleteThing = createMutationStore<{ id: string }[]>(
  async ({ data: selectedOptions }) => {
    return Promise.all(
      selectedOptions.map((opt) =>
        // That should be a direct API call, not mutation call
        deleteThing({ thingID: opt.id }).catch(() => {
          // There you have it! That's your per-operation `onError` right there.
        })
      )
    );
  }
);

So I think your particular use case is more or less covered by current API. You just need to use promises creatively.

@dkzlv
Copy link
Member Author

dkzlv commented Mar 6, 2024

@martinmckenna Btw, after giving it a good thought, I think that your specific code will break in some of future releases 😬 Read issue #34 I just created for more details.

@dkzlv dkzlv mentioned this issue Apr 3, 2024
7 tasks
@dkzlv dkzlv closed this as completed in #36 Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants