Promise continues after error

Question

I have some async code that needs to stop in case of error but keeps executing:

async saveCoupons({ state, rootState, dispatch, commit }) {
    const promises = []
    state.userCoupons.forEach(coupon => { 
        if (coupon.isNew && coupon.isUpdated) {
            // if the user is creating a new coupon
            promises.push(Vue.axios.post('/api_producer/coupons.json', coupon, { params: { token: coupon.token } }))
        } else if (!coupon.isNew && coupon.isUpdated) {
            // if the user is updating the coupon
            promises.push(Vue.axios.patch(`api_producer/coupons/${coupon.id}/`, coupon, { params: { token: coupon.token } }))
        }
    })
    try {
        await Promise.all(promises)
        dispatch('utilities/showModal', 'success', { root: true })
        dispatch('fetchProducerCoupons')
    } catch (err) {
        let couponToken = err.request.responseURL.split('token=')[1]
        commit('ADD_ERROR_ON_COUPON', couponToken)
        console.log(err)
    }
}

This is how the code is currently structured, it works, but I realize it's terrible. What I need to do is stop the excution of

dispatch('utilities/showModal', 'success', { root: true })
dispatch('fetchProducerCoupons')

In case one of the api calls fails. I wanted to catch the error inside the forEach so I already have the item available and I can add the error to it right away as opposed to doing it after (which is what I'm doing now with { params: { token: coupon.token } }.

Do the requests need to be fired in parallel or could they be fired after another? — Moritz Schmitz v. Hülst, Feb 06 '18 at 20:58
No it doesn't really matter. As long as I can catch the error of the call right from within the forEach, so I can attach the error to the item and than display it in the front end. If it does success than all it's doing is showing success modal and than refreshing the items by fetching them again — Giacomo, Feb 06 '18 at 21:01
Add a .catch to each one of the Promise calls and "throw" the parameters you need to your error handler. — Luiz Chagas Jr, Feb 06 '18 at 21:04
@LuizChagasJr it's still executing the rest of the code at the bottom this way, unless I'm doing something wrong. Could you actually write it down please? — Giacomo, Feb 06 '18 at 21:06
If any of the `promises` is rejected, then the `await` will trigger an exception and your dispatches won't be executed. Are the promises not rejected when the api calls fail? — Bergi, Feb 06 '18 at 21:18
I would just make them run one after another with array reduce. (google: reduce promise), that means as soon as one fails you get that error. — Bergur, Feb 06 '18 at 21:19

Moritz Schmitz v. Hülst · Accepted Answer · 2018-02-06T21:38:44.953

2

I think the best way would be to wrap the Vue.axios requests into your own Promise. Then, if the requests fail, you have the coupon tokens in your error.

Something like

const promises = [];

promises.push(
  Vue.axios.post('/api_producer/coupons.json', coupon)
    .catch(() => { throw new Error(coupon.token) }));
    
Promise.all(promises).catch(tokens => {
  tokens.forEach(token => {
    // Commit/handle errorous token
  });
});

edited Feb 06 '18 at 21:38

answered Feb 06 '18 at 21:15

Moritz Schmitz v. Hülst

3,229
4
36
63

Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Feb 06 '18 at 21:19
Yes this works. I'm only using `catch` in the api call, as I don't need to do anything on success, and in the `Promise.all(promises)` i have the `then()` to execute the 2 functions at the end of the code. I will post again what I have. I'm wondering if there's a way to make all of this using await/async, that I like more than Promises – Giacomo Feb 06 '18 at 21:25
@Bergi, what do you mean? – Moritz Schmitz v. Hülst Feb 06 '18 at 21:27
@Giacomo, yes of course you could do that. You just need a 'global' variable which will tell you in the end, whether any call failed or not and only then execute your `dispatch` actions. – Moritz Schmitz v. Hülst Feb 06 '18 at 21:29
@Bergi, okay, I understand what you mean. Just as I understood OP, he wants to access the `coupon.token` in the catch. However, if the `Vue.axios` request fails, he still won't know which token it was. Therefore I wrapped it into another Promise. Or is this handled by `Vue.axios`? – Moritz Schmitz v. Hülst Feb 06 '18 at 21:32
It's fine to produce a transformed promise, but it's not fine to wrap it in `new Promise` for that. Just do `promises.push(axios.post(…).chain(() => { throw new Error(coupon.token); }))` – Bergi Feb 06 '18 at 21:34
@Bergi, gotcha. – Moritz Schmitz v. Hülst Feb 06 '18 at 21:35
Fixed. Not using the constructor anymore, but throwing an error so the promise can be chained further. – Moritz Schmitz v. Hülst Feb 06 '18 at 21:40

sliptype · Answer 2 · 2018-02-06T21:08:35.573

0

You can wrap your api call in another promise and check the status. Something like this:

promises.push(

  new Promise((resolve, reject) => {

    Vue.axios.post('/api_producer/coupons.json', coupon, { params: { token: coupon.token } })
    .then((response) => {
      if (response.status !== 200) {
        coupon.error = true;
        reject();
      } else {
        resolve();
      }
    });

  })
);

The reject will keep these two lines from being executed:

  dispatch('utilities/showModal', 'success', { root: true })
  dispatch('fetchProducerCoupons')

edited Feb 06 '18 at 21:08

answered Feb 06 '18 at 20:56

sliptype

2,804
1
15
26

1

but this still doesn't allow me to get access to the error right in the forEach, it has to be outside – Giacomo Feb 06 '18 at 20:57
ahh!! I tried that too but i was missing the `new Promise`, not understanding why that's the solution now but it's working – Giacomo Feb 06 '18 at 21:16
1

Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Feb 06 '18 at 21:19
I didn't know this was an antipattern, thanks for bringing it to my attention – sliptype Feb 06 '18 at 21:57

Giacomo · Answer 3 · 2018-02-06T21:55:07.040

0

Thanks to Moritz Schmitz v. Hülst & sklingler93 for the help, I restructured the code and it's working.

I'm wondering if there's a way to write all of this using only async/await... If anybody has an idea, would love to see it :)

saveCoupons({ state, rootState, dispatch, commit }) {
    const promises = []
    state.userCoupons.forEach(coupon => {          
        if (coupon.isNew && coupon.isUpdated) {
            // if the user is creating a new coupon
            promises.push(new Promise((resolve, reject) => {
                Vue.axios.post('/api_producer/coupons.json', coupon)
                    .then(response => resolve(response))
                    .catch(err => {
                        reject(err)
                        commit('ADD_ERROR_ON_COUPON', coupon.token)
                    })                        
            }))
        } else if (!coupon.isNew && coupon.isUpdated) {
            // if the user is updating the coupon
            promises.push(new Promise((resolve, reject) => {
                Vue.axios.patch(`api_producer/coupons/${coupon.id}/`, coupon)
                    .then(response => resolve(response))
                    .catch(err => {
                        reject(err)
                        commit('ADD_ERROR_ON_COUPON', coupon.token)
                    })
            }))
        }
    })
    Promise.all(promises)
        .then(() => {
            dispatch('utilities/showModal', 'success', { root: true })
            dispatch('fetchProducerCoupons')
        })
        .catch(err => console.error(err))
},

edited Feb 06 '18 at 21:55

answered Feb 06 '18 at 21:27

Giacomo

1,056
4
16
24

This code cannot work. The promises in your array are never settled – Bergi Feb 06 '18 at 21:32
Isn't `Promise.all(promises)...` taking care of that? @Bergi – Giacomo Feb 06 '18 at 21:33
No. `Promise.all` does not settle any promises other than the one it returns. In all of those `new Promise` executor callbacks, you never call `resolve` or `reject`! – Bergi Feb 06 '18 at 21:35
Ok I added `.then(response => resolve(response))` in each new Promise. Is that it? – Giacomo Feb 06 '18 at 21:37
[No](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it). Now you're just ignoring errors. – Bergi Feb 06 '18 at 21:38
@Bergi updated again, the catch. Was that it? If not I'm really not understanding – Giacomo Feb 06 '18 at 21:53
Better, but still employing the antipattern. Btw, do you want to `commit` for each error that happens or only once? – Bergi Feb 06 '18 at 21:54
@Bergi commit for each error because I than show it inside the actual element in the html. Let's say I have 3 boxes with the coupon, one of them is updated, user clicks the save (a general button for all of them). The forEach goes through them and checks if the property of `isUpdated` is true it makes a patch request, but in case the coupon name was already used by another user it throws the error and I need to display it inside the box. Otherwise if there are no errors, everything is good, shows success modal, fetch coupons again and that's it. Makes sense? – Giacomo Feb 06 '18 at 21:57

Promise continues after error

3 Answers3

Linked