nanostores / query

⚡️ Powerful data fetching library for Nano Stores. TS/JS. Framework agnostic.
MIT License
216 stars 10 forks source link

Handling mutation errors on per-store level #28

Closed dkzlv closed 5 months ago

dkzlv commented 8 months ago

First mentioned in #27.

The proposed solutions are:

martinmckenna commented 7 months ago

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 commented 7 months ago

@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 commented 6 months ago

@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.