-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Conversation
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
There was a problem hiding this 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 👍
test/unit/modules.spec.js
Outdated
const afterSpy = jasmine.createSpy() | ||
const store = new Vuex.Store({ | ||
actions: { | ||
[TEST]: () => new Promise(resolve => resolve()) |
There was a problem hiding this comment.
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()
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh duh. Will update!
Also, can we update TypeScript type declaration? If you are not sure how to update it, I'll do it later. |
Generalize subscribeAction's type declaration to accept both a function or an object and add type tests for that
@ktsn I have no experience with typescript, but the syntax seemed straightforward enough. Let me know if you have suggestions! |
@wa3l Well done! 👍 |
@ktsn glad to hear it! Next steps? |
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? |
Waiting for @yyx990803's review. |
@yyx990803 any updates on this? |
Guys (@ktsn @yyx990803), please, if you don't want to approve this request, can you suggest some other solution? |
@yyx990803 please, It is very useful to me, waiting for you review |
@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... |
This would be really useful for integration tests too. Right now I wish I could have something like closeDialog();
waitForStoreActions();
expect(...); The FWIW, I would have just expected 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. |
Looks like that it's done to be merged :) |
@yyx990803 any timeline on merging this? |
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! |
@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. |
I will say this implementation is SUPER important and should have higher priority. WHY? Because there is a plan to merge Let's say for some reason (e.g. server response 401), an action called |
@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? |
Over a year since this was originally proposed? |
Sorry for letting this sit, will do a release next week. |
Thank you so much for the quick response! Excited for my loading screens! |
This reverts commit 76818c1.
@yyx990803 any updates on the release? |
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 tosubscribeAction
then it's assumed that it's the before subscriber for backwards compatibility.after
: is called after the action resolves.Examples:
Let me know if you have any suggestions/improvements!