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

Action before and after subscribers #1115

Merged
merged 5 commits into from Jan 4, 2019
Merged

Conversation

wa3l
Copy link
Contributor

@wa3l wa3l commented Jan 3, 2018

See issue #1098. Thanks to @ktsn for proposing the api.

Add the ability to specify before and after action subscribers that behave as follows:

  • before: is called before the action like subscribers behave now. If you only pass a function to subscribeAction then it's assumed that it's the before subscriber for backwards compatibility.
  • after: is called after the action resolves.

Examples:

// calls foo() before every action
store.subscribeAction(foo)

// calls foo() before every action
store.subscribeAction({
  before: foo
})

// calls foo() after an action resolves
store.subscribeAction({
  after: foo
})

// calls foo() before every action and bar() after the action resolves
store.subscribeAction({
  before: foo,
  after: bar,
})

Let me know if you have any suggestions/improvements!

Wael Al-Sallami added 2 commits January 2, 2018 16:52
Action subscribers are called before the action by default. This allows them to specify before and after subscribers where the after subscriber is called when the action resolves
make sure that the before subscriber is called before the action, while the after subscriber is called after it resolves
@wa3l wa3l changed the title Action sub waiting Action before and after subscribers Jan 3, 2018
Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

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

Other than a small nitpicking, looks good to me 👍

const afterSpy = jasmine.createSpy()
const store = new Vuex.Store({
actions: {
[TEST]: () => new Promise(resolve => resolve())
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified with Promise.resolve() 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh duh. Will update!

@ktsn ktsn requested a review from yyx990803 January 3, 2018 12:22
@ktsn
Copy link
Member

ktsn commented Jan 3, 2018

Also, can we update TypeScript type declaration?
https://github.com/vuejs/vuex/blob/dev/types/index.d.ts#L20

If you are not sure how to update it, I'll do it later.

Wael Al-Sallami added 3 commits January 3, 2018 12:02
@wa3l
Copy link
Contributor Author

wa3l commented Jan 3, 2018

@ktsn I have no experience with typescript, but the syntax seemed straightforward enough. Let me know if you have suggestions!

@ktsn
Copy link
Member

ktsn commented Jan 4, 2018

@wa3l Well done! 👍

@wa3l
Copy link
Contributor Author

wa3l commented Jan 4, 2018

@ktsn glad to hear it! Next steps?

@ktsn ktsn added the proposal label Jan 11, 2018
@KindWizzard
Copy link

Hi guys! How are things going with this improvement? The last comment was added 25 days ago :) Looks like it's freezed. Will it be released?

@ktsn
Copy link
Member

ktsn commented Jan 30, 2018

Waiting for @yyx990803's review.

@wa3l
Copy link
Contributor Author

wa3l commented Mar 14, 2018

@yyx990803 any updates on this?

@KindWizzard
Copy link

KindWizzard commented Apr 3, 2018

Guys (@ktsn @yyx990803), please, if you don't want to approve this request, can you suggest some other solution?

@Vincent1993
Copy link

@yyx990803 please, It is very useful to me, waiting for you review

@wa3l
Copy link
Contributor Author

wa3l commented May 8, 2018

@ktsn is there no way around Evan having to review every single PR? Seems like there is enough appetite for this PR but I don't see it getting merged if one person has to review every pull request on several different libraries...

@chearon
Copy link

chearon commented Jun 5, 2018

This would be really useful for integration tests too. Right now I wish I could have something like

closeDialog();
waitForStoreActions();
expect(...);

The waitForStoreActions() part could be implemented by using this PR.

FWIW, I would have just expected subscribeAction to be changed to have a promise as the third argument:

store.subscribeAction(async (action, state, done) => {
  // this is already like the before() part of the PR
  await done;
  // now this is like the after() part of this PR
});

It might be a breaking change though since it has to be called after all the actions are fired.

@marlonbarcarol
Copy link

Looks like that it's done to be merged :)

@wa3l
Copy link
Contributor Author

wa3l commented Sep 12, 2018

@yyx990803 any timeline on merging this?

@leoyli
Copy link

leoyli commented Oct 25, 2018

Hi @yyx990803, @ktsn, I'm just wondering if anything have to be done in order to pass this through. Or is this feature will have some other concerns (e.g. conflicts with the future movement).

I can see the last commit is done in very early this year and have been fully reviewed in June, but still it is not merged and sitting there. Please, please, please pointing some directions, concerns, or whatever so the community become more healthy and moving. I definitely don't know why or if something haven't been done. May be the documentation or using example? At least the big heads have to tell us so we can contribute. Thanks!

@wa3l
Copy link
Contributor Author

wa3l commented Oct 29, 2018

@leoyli I'm equally frustrated. I don't know if what happened with this PR is typical of other ones, but it certainly discouraged me from contributing any further. I've used Vue (and vuex, vue-router, vue-meta) everyday for the past couple of years, so I'm happy to pay some of that karma back to the community, but I just don't see how that's possible with the current process.

KindWizzard pushed a commit to KindWizzard/vuex that referenced this pull request Oct 29, 2018
@leoyli
Copy link

leoyli commented Oct 30, 2018

I will say this implementation is SUPER important and should have higher priority. WHY? Because there is a plan to merge mutations in to actions in Vuex 3, and we need a mechanism to kick the subscriber before and after the action be executed. In our architecture used in the project, router is the outermost layer and we don't want them to be owned/operated in terms of vm.$router inside the view model.

Let's say for some reason (e.g. server response 401), an action called ACTION_LOGOUT was fired by another action that performs an AJAX call, so we want our router to be reactive to this action and redirect the user to the login page. Since one of the key step is the action committing the store to remove the authentication token inside this action, we want the router can spontaneously response it only after this action have been fulfilled. In such case, the push of subscription have to be done later. So far, this only can achieved by subscribe that particular mutation, but this way is not so clean per se and will be unavailable in VueX 3...

@ghost
Copy link

ghost commented Dec 10, 2018

@ktsn Based on your comment (#1115 (comment)) this awaited review by Evan who approved this in June (#1115 (review)). Could you please comment on the current state of this pull request?

@Sunhat
Copy link

Sunhat commented Jan 4, 2019

Over a year since this was originally proposed?

@yyx990803 yyx990803 merged commit 76818c1 into vuejs:dev Jan 4, 2019
@yyx990803
Copy link
Member

Sorry for letting this sit, will do a release next week.

@Sunhat
Copy link

Sunhat commented Jan 4, 2019

Thank you so much for the quick response! Excited for my loading screens!

lishihong added a commit to lishihong/vuex that referenced this pull request Jan 10, 2019
@wa3l
Copy link
Contributor Author

wa3l commented Jan 15, 2019

@yyx990803 any updates on the release?

@yyx990803
Copy link
Member

Out in https://github.com/vuejs/vuex/releases/tag/v3.1.0

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.

None yet

9 participants