timolins / react-hot-toast

Smoking Hot React Notifications 🔥
https://react-hot-toast.com
MIT License
9.68k stars 323 forks source link

Awaiting on the toast.promise throws #157

Open franciscop-sc opened 2 years ago

franciscop-sc commented 2 years ago

This is a feature request based on some surprising behavior I found. Alternatively, a documentation request to make said behavior clear. I can help with either, but I'd like to know what @timolins and the library authors prefer to do first 😃

I expect that if the promise fails, then toast.promise should handle it internally. And it does in this case, since internally it calls .catch():

const bakeToast = async () => {
  toast.promise(Promise.reject("Hey..."), {
    loading: "Loading",
    success: "Got the data",
    error: "Error when fetching"
  });
};

But notably, if we await for that await toast.promise(...), now it throws! This will show the alert():

const bakeToast = async () => {
  try {
    await toast.promise(Promise.reject("Hey..."), {
      loading: "Loading",
      success: "Got the data",
      error: "Error when fetching"
    });
  } catch (error) {
    alert("Should not get here!");
  }
};

Unfortunately this behavior is surprising IMHO, the toast should either handle errors in all cases (better) or throw errors in all cases (not preferred), whether we wait for it or not. Is the role of the popup considered as "error handled"? I'd say yes, since there's a explicit "error: ..." key there, so I'd expect that the second example not to throw the error.

This happens because toast.promise() conveniently returns a promise to know when it's finished. IMHO this promise should always resolve positively, since the error is already handled in the toast itself, and be used only for timing externally. So I'd change this code:

  promise
    .then((p) => {
      toast.success(resolveValue(msgs.success, p), {
        id,
        ...opts,
        ...opts?.success,
      });
      return p;
    })
    .catch((e) => {
      toast.error(resolveValue(msgs.error, e), {
        id,
        ...opts,
        ...opts?.error,
      });
    });

  return promise;
};

For this one, so that if the promise has been caught then it will become resolved instead of rejected:

  return promise
    .then((p) => {
      toast.success(resolveValue(msgs.success, p), {
        id,
        ...opts,
        ...opts?.success,
      });
      return p;
    })
    .catch((e) => {
      toast.error(resolveValue(msgs.error, e), {
        id,
        ...opts,
        ...opts?.error,
      });
    });
};
TylerAHolden commented 2 years ago

I have a similar situation. I would love to be able to turn off either the error message or have it catch the error so that it doesn't propagate. I can see use cases where you want both so I'm not sure the best idea is to ALWAYS resolve the promise successfully. But my particular flavor of function code feels like it needs some hacky-ness to handle this where an option flag might help me out.

Example of code:

const handleSaveObject = async () => {
    try {
      if (!somevalue) throw new Error('Uh oh no value found');
      if (!someOthervalueCheck) throw new Error('We do not like this value');

      await toast.promise(
        updateObject(someValue),
        {
          loading: 'Saving...',
          success: 'Saved!',
          error: (err) => err.message,
        }
      );
    } catch (err) {
      toast.error(err.message);
    }
  };

In this case, if updateObject throws an error 2 toasts will be shown because the updateObject function would be thrown sending the error to my catch block as well as the toast.promise showing the error message. Am I just missing a good way to handle this simply without adding another try/catch?

TylerAHolden commented 2 years ago

I'd be happy to contribute but I'm not sure what the best approach would be and what the default looks like. Maybe a stopErrorPropogation value in the options? or maybe if the error ValueOrFunction option is set to undefined/null, it rejects the error and if it's provided, then it "catches" the error and resolves successfully? I'm thinking the latter might make the most sense. If you don't provide the "settings" to handle the error then it essentially won't handle the error.

franciscop-sc commented 2 years ago

As a quick workaround, I've started to do this, which looks pretty wrong:

await toast.promise(...).catch((err) => null);
// Error IS handled, please see https://github.com/timolins/react-hot-toast/issues/157

However if I was reviewing code and someone else wrote that, and I didn't know how the promise() works internally (it does both, handle the error AND throw the error) I'd review the code wrongly as "please don't ignore errors".