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

Return original Action Promise instead of pipe after "catch" when registerAction #1456

Open
wants to merge 2 commits into
base: dev
from

Conversation

Projects
None yet
2 participants
@isnifer

isnifer commented Nov 20, 2018

Hello everyone!

Prerequisites

In my project, I do not use just a promise to do actions.
Simplified version looks like this:

class CustomPromise {
  constructor({ promise }) {
    this.promise = promise.then(this.handleSuccess, this.handleError)
  }

  handleSuccess = () => {
    // ...
  }

  handleError = () => {
    // ...
  }

  handleCancel = () => {
    // ...
  }

  then = (success, error) => this.promise.then(success, error)

  catch = error => this.promise.catch(error)
}

When I wanted to explicitly work with my action promise I found that I got only PromiseValue instead of my CustomPromise.
I had a vue extenstion for Chrome (yes, I removed it to test and verify a bug).

Finally

This line creates a promise chain that not only subscribes to catch. This line provides only promise value/error to userland code instead of an original promise.

How to fix?

Developer tools may be a useful tool, but Vuex should not change itself behavior when Devtools are available.
This catch should ONLY subscribes to errors (creates a parallel pipe and DO NOT THROW an error inside).
A userland code also will catch an error.

@isnifer

This comment has been minimized.

isnifer commented Nov 20, 2018

@ktsn please, take a look :)

@ktsn

This comment has been minimized.

Member

ktsn commented Nov 26, 2018

The change which makes action handlers not return chained promise looks good to me. But I'm not sure it is a good idea that the user rely on the behaviour. Because it still can be the native promise when you have multiple actions with the same name and it is not explicitly obvious to the users.

@ktsn

ktsn approved these changes Nov 26, 2018

@isnifer

This comment has been minimized.

isnifer commented Nov 26, 2018

@ktsn I think when a developer creates multiple actions with the same name — he knows what he is doing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment