championswimmer / vuex-module-decorators

TypeScript/ES7 Decorators to create Vuex modules declaratively
https://championswimmer.in/vuex-module-decorators/
MIT License
1.8k stars 170 forks source link

Single commit/dispatch in @Action called only. #37

Closed m-ripper closed 5 years ago

m-ripper commented 6 years ago

In the official Vuex-documentation it is stated, that multiple commits in a single action are possible. For example here:

actions: {
  checkout ({ commit, state }, products) {
    // save the items currently in the cart
    const savedCartItems = [...state.cart.added]
    // send out checkout request, and optimistically
    // clear the cart
    commit(types.CHECKOUT_REQUEST)
    // the shop API accepts a success callback and a failure callback
    shop.buyProducts(
      products,
      // handle success
      () => commit(types.CHECKOUT_SUCCESS),
      // handle failure
      () => commit(types.CHECKOUT_FAILURE, savedCartItems)
    )
  }
}

Now when I tried to get a similar behavior with this neat module, only the first commit/dispatch gets called and the action ends afterwards. Here is an example:

    @Mutation
    public [PURGE_AUTH]() {
        this.token = '';
        this.user = {id: '', username: ''};

        localStorage.removeItem('token');
        delete axios.defaults.headers.common['X-AUTH-TOKEN'];
    }

    @Action
    public async [LOGOUT]() {
        await doLogout();
        console.log('COMMITTING');
        this.context.commit(PURGE_AUTH);
        console.log('DISPATCHING');
        this.context.dispatch(RESET_ALL);
        console.log('AFTER DISPATCH');
        return;
    }

Only 'COMMITTING' will be printed to the console and the the other dispatches/commits do not get called. I have also tried to use the await keyword for the commits but that does not do anything.

championswimmer commented 6 years ago

can you make a PR with a test that fails on this case

m-ripper commented 6 years ago

I found a work-around for the issue, I don't know if it is considered hacky or not but that's how I solved the problem:

    @Action({rawError: true})
    public async [CHECK_AUTH]() {
        const context = this.context;
        const resp = await doCheckAuth();

        if (typeof resp.data.error !== 'undefined') {
            try {
                context.commit(PURGE_AUTH);
            } catch (e) {
                // whatever is necessary
            }
            context.commit(RESET);
        } else {
            context.commit(SET_AUTH, resp.data);
        }
    }

I know this code is a little different than above but I think one might get my point. I realized after doing a few tests, that the reference to context was lost as soon as something like await function() was called, meaning that I could not commit, nor dispatch via this.context. By setting a reference to the context I managed to do those commits and dispatches.

I would also recommend to enable rawError to be able to catch errors that could occur in commits/dispatches.

I will look whether I find the time to make a PR, but maybe for now this is enough.

kakenbok commented 6 years ago

The reference to "context" gets lost using await/promise, this is true, and this is the real bug behind. @Veake would you mind to add a failing test and make a pull request?

m-ripper commented 6 years ago

I made a small test but it seems that it is working fine there. I can make a PR with it if you want, but since the test seems to work, my guess is that there was an error in code somewhere.

championswimmer commented 6 years ago

Yeah a failing test would help

On Sun 21 Oct, 2018, 9:46 PM Veake, notifications@github.com wrote:

I made a small test but it seems that it is working fine there. I can make a PR with it if you want, but since the test seems to work, my guess is that there was an error in code somewhere.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/championswimmer/vuex-module-decorators/issues/37#issuecomment-431682010, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQ_ymnSYhUxi1dGB-yV5hSj0B34nfeVks5unJ3sgaJpZM4XddG9 .

rumbcam commented 5 years ago

In pull request #55 I've added tests that reproduce similar symptoms as what is described in this pull request as well as a fix for it too.

FlorianWendelborn commented 5 years ago

@rumbcam @championswimmer @Veake since #55 was merged, can this issue be closed?

m-ripper commented 5 years ago

Yes the issue has been solved for me.