Skip to content
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

Allow action subscribers to catch rejections. #1531

Closed
wants to merge 3 commits into from

Conversation

jmplahitko
Copy link
Contributor

Enhancement request to allow Vuex plugins to subscribe to rejected action promises.

@kiaking
Copy link
Member

kiaking commented Apr 21, 2020

This is kinda interesting. Could you elaborate a bit more on specific use case for the feature? What might plugin just do by catching this error instead of letting users handle it?

@jmplahitko
Copy link
Contributor Author

jmplahitko commented Apr 22, 2020

@kiaking - Sure. Since actions are asynchronous, it's helpful to delegate tracking of which actions are "outstanding" to an outside source. This can drive a number of UI features like spinners, progress bars, etc, especially when actions ultimately result in an http request that blocks user interaction: "While this action is outstanding, do this thing (spin, etc..)". Vuex plugins would work great for this, except for when http calls (in this use case) result in a rejection. The action never gets resolved as far as the plugin is concerned. While rejections could be handled by the caller, this gets unwieldy to do on a case-by-case basis in a large system.

Aside from this use case, it seemed like a missed opportunity to allow for subscribers to know when an action resolve, and not when it rejects. It implies that subsequent events only need to occur on the happy path, and not on the sad path.

I can discuss further if needed.

@kiaking
Copy link
Member

kiaking commented Apr 23, 2020

Thank you for the detailed explanation! Yeah it make sense. Also there was other issues wanting to have this feature as well.

@ktsn What do you think? I think it would be nice have a dedicated error handling hook on subscribeAction. If this is the way to go, I think we can further discuss on implementation details (like maybe we shouldn't throw at the end to avoid a braking change).

@ktsn
Copy link
Member

ktsn commented Apr 23, 2020

Yeah, this is something we should support, I think.

Copy link
Member

@kiaking kiaking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmplahitko Please remove dist folder from the commit. Also, could you check the comment I made?

src/store.js Outdated
console.error(_e)
}
}
throw e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not throw here since this is technically a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiaking - I was not aware this was a breaking change, as existing tests pass with the throw. Without throwing here, the catch handler on the original dispatch call does not get called, which actually breaks an existing test in store.spec.js 'detect action Promise errors'. Am I missing something here? Let me know how to proceed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiaking - Understand the dist folder, but please advise on the comment I added after yours regarding the throw.

@jmplahitko
Copy link
Contributor Author

@kiaking - I figured out how we can keep dispatch catch handlers in tact and not throw the error, but I got a little confused about removing dist from the commit 🗡️ - Is there a particular git strategy you prefer for this?

Copy link
Member

@kiaking kiaking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 2 more comments due to some change in the latest commit.

.filter(sub => sub.after)
.forEach(sub => sub.after(action, this.state))
} catch (e) {
if (process.env.NODE_ENV !== 'production') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change process.env.NODE_ENV !== 'production' to __DEV__. This change was introduced in the latest commit.

Suggested change
if (process.env.NODE_ENV !== 'production') {
if (__DEV__) {

.filter(sub => sub.catch)
.forEach(sub => sub.catch(action, this.state, e))
} catch (_e) {
if (process.env.NODE_ENV !== 'production') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one as well.

Suggested change
if (process.env.NODE_ENV !== 'production') {
if (__DEV__) {

@kiaking
Copy link
Member

kiaking commented Apr 30, 2020

I figured out how we can keep dispatch catch handlers in tact and not throw the error, but I got a little confused about removing dist from the commit 🗡️ - Is there a particular git strategy you prefer for this?

Thanks a lot! You could revert the commit by cherry picking, but, as I commented about some latest change, it might be cleaner to create a new PR from the latest dev branch.

@kiaking
Copy link
Member

kiaking commented Apr 30, 2020

@jmplahitko Sorry for splitting the comments. Also could you rename catch to error? There was a discussion about the naming for the option at #1558.

const waitPlugin = (store) => {
  store.subscribeAction({
    before: (action) => {
      console.log(action.type);
    },
    after: (action) => {
      console.log(action.type);
    },
    error: (action, error) => {
      console.log(action.type);
    }
  });
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants