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 subscribers do not wait for associated actions to resolve #1098

Closed
wa3l opened this issue Dec 20, 2017 · 9 comments
Closed

Action subscribers do not wait for associated actions to resolve #1098

wa3l opened this issue Dec 20, 2017 · 9 comments

Comments

@wa3l
Copy link
Contributor

wa3l commented Dec 20, 2017

What problem does this feature solve?

I noticed that my store action subscribers were being called before the actions finish. Digging into the code, it looks like not only do action subscribers not wait on the actions to resolve, but they execute before the action. Mutation subscribers, on the other hand, seem to work as expected (call mutation first, call subscriber second). Since actions are often asynchronous (and vuex always wraps them with promises), I think it makes more sense to wait on the action and then call subscribers.

I wasn't sure if this was intentional or not, hence the feature request.

What does the proposed API look like?

I think something like the following would be more in line with what developers expect:

const result = entry.length > 1
      ? Promise.all(entry.map(handler => handler(payload)))
      : entry[0](payload);
result.then(() => this._actionSubscribers.forEach(sub => sub(action, this.state)));
return result;

I could open a pull request if that looks reasonable (will add tests, etc.)

@LinusBorg
Copy link
Member

If you recap the discussions in #960 -> #929 -> #908 , the use case of this feature was to get informed of actions when they are dispatched, not some (maybe very long) time later when they have finished.

What you want is probably something else.

@wa3l
Copy link
Contributor Author

wa3l commented Dec 27, 2017

@LinusBorg interesting, thanks for providing that context. I'm trying to implement undo/redo, which a bit different than logging in that mutations represent a state change, but actions represent the actual "history steps" in state that the user cares about undoing. Without a way to notify some centralized subscriber when an action resolves, there is no built-in way of tracking those "logical" history changes.

I can probably wrap all (or a subset) of my actions with a function that does the waiting/reporting for me. I saw the term "action enhancer" thrown around a couple of times, but the vuexfire docs weren't very clear. The problem with this solution is, after refactoring my existing store, if anyone in the future forgets or doesn't know to wrap their actions with this action wrapper, then they are suddenly excluded from the history tracking.

I understand that subscribeAction was not meant for this purpose, but would it make sense to create an asyncActionSubscribe() that does what I'm describing?

@ktsn
Copy link
Member

ktsn commented Dec 28, 2017

I think extending subscribeAction to let the store notify after finished action would work.

For example:

store.subscribeAction({
  before () { /* called before dispatching action */ },

  after () { /* called after the returned Promise is resolved */ }
})

@ktsn ktsn added the proposal label Dec 28, 2017
@wa3l
Copy link
Contributor Author

wa3l commented Jan 2, 2018

@ktsn that would be awesome. I'll take a crack at it.

wa3l pushed a commit to wa3l/vuex that referenced this issue Jan 3, 2018
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
wa3l pushed a commit to wa3l/vuex that referenced this issue Jan 3, 2018
make sure that the before subscriber is called before the action, while the after subscriber is called after it resolves
wa3l pushed a commit to wa3l/vuex that referenced this issue Jan 3, 2018
wa3l pushed a commit to wa3l/vuex that referenced this issue Jan 3, 2018
Generalize subscribeAction's type declaration to accept both a function or an object and add type tests for that
@johnmerced-ks
Copy link

Looking forward for this! Any idea when will this be released?

@wa3l
Copy link
Contributor Author

wa3l commented Jan 31, 2018

@johnmerced the pr is here, waiting on Evan's review.

@johnmerced-ks
Copy link

johnmerced-ks commented Jan 31, 2018

@wa3l Awesome! We are looking forward for this! 👍 Thank you!

@jbgraug
Copy link

jbgraug commented Oct 24, 2018

Any updates or deadline on this feature?

yyx990803 pushed a commit that referenced this issue Jan 4, 2019
* allow action subscribers to specify before/after hooks (#1098)

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

* add test cases for the new before/after action subscribers (#1098)

make sure that the before subscriber is called before the action, while the after subscriber is called after it resolves

* Replace Promise initialization with shorter form (#1098)

* Update subscribeAction type declaration and add type tests (#1098)

Generalize subscribeAction's type declaration to accept both a function or an object and add type tests for that
@winniehell
Copy link

@ktsn #1115 is merged and released, so this issue can probably be closed.

@wa3l wa3l closed this as completed Jun 7, 2019
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

No branches or pull requests

6 participants