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

[atom] Async effects in events bus #14

Closed
allforabit opened this issue Mar 7, 2018 · 7 comments
Closed

[atom] Async effects in events bus #14

allforabit opened this issue Mar 7, 2018 · 7 comments

Comments

@allforabit
Copy link
Contributor

This is related to the previous issue but it's probably separate enough to warrant it's own issue.

How would you go about adding an async event such as an an ajax request? I can't see a way to do it via the event handler config because they don't get a handle on the event bus. They only get the state and event. I can see that you could do it after the event bus is instantiated but it would be nice to be able to be able to do it through the event config. It would probably need to be another built in effect similar to "dispatch" that might look like this: [FX_DISPATCH_ASYNC]: {effect: [FETCH_JSON: {url: "http://thi.ng/feed.json"}], handler: "myHandler"}. The effect function would then need to take a callback as an argument to callback into the event system.

I can attempt a pull request for this!

@postspectacular
Copy link
Member

Hey, sorry just briefly... I already noticed this too and the current lack of getting a handle on the bus itself from a handler is an oversight of my possibly too speedy refactoring last night. My original solution which I used in existing projects was to have an InterceptorContext, a kind of accumulator object into which handlers would not just add their side effects, but which also allows access to other bindings (like the bus, or other services). I got rid of this last night after reading about & liking redux's approach to just return values (instead of assigning them to the accumulator), but I think I might do a U-turn on this again... also still have to read about Citrus... so many links, so little time ;) Thanks, for these pointers though!

You also correctly observed that the way the "addCounter" effect in the demo is defined from within the app() was exactly for that reason. I like your FX_DISPATCH_ASYNC idea too. You can try it out first by just adding it as effect in "userland". Might be completely sufficient. If you feel like doing PR, by all means! Thanks

@postspectacular
Copy link
Member

So I just had a quick go at this and was wondering what your thoughts are :)

New demo: http://demo.thi.ng/umbrella/async-effect/
Source: https://github.com/thi-ng/umbrella/blob/master/examples/async-effect/src/index.ts

@postspectacular
Copy link
Member

@allforabit i hope you don't mind, but since i've already got the demo and done some more manual testing and updating other smaller things, I'll cut another release with this promise based version of the FX_DISPATCH_ASYNC from the demo. Hope you didn't start working the PR for this yet...

@postspectacular
Copy link
Member

postspectacular commented Mar 8, 2018

Related, I also updated the SideEffect type sig to accept the EventBus as optional argument for other use cases where this is needed/useful (e.g. see the addCounter effect from the earlier demo). This still keeps the event handlers nicely pure, but allows effects to take more control.

@allforabit
Copy link
Contributor Author

That's absolutely perfect. It's much nicer to do the normal dispatch along with an async dispatch as it's not nearly as nested this way. I was thinking that it'd be good to pass in the dispatch function to the effect handlers but it's probably the most flexible way is to pass in the full event bus. I can envisage standard effect handlers for channels and the likes, that could do multiple dispatches as new values come in. This would work nicely for socket connections for instance. Brilliant stuff, thanks very much for that!

@postspectacular
Copy link
Member

Yeah, exactly & thanks to you, sir! Regarding multiple dispatches, I also added support for each individual interceptor to have the option to declare multiple side effects of same type:

https://github.com/thi-ng/umbrella/blob/master/packages/atom/src/event-bus.ts#L269

@postspectacular
Copy link
Member

I believe it's safe to close this issue for now...

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

No branches or pull requests

2 participants