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

Make the event channel tappable so that events can be inspected by third parties. #118

Closed
wants to merge 1 commit into from
Closed

Make the event channel tappable so that events can be inspected by third parties. #118

wants to merge 1 commit into from

Conversation

pupeno
Copy link
Contributor

@pupeno pupeno commented Sep 23, 2015

The goal of this is to implement prerendering that needs visibility into what's going on in re-frame. I created a library called com.carouselapps/prerenderer that uses this feature. You can learn more about it here: https://carouselapps.com/prerenderer/

@thenonameguy
Copy link
Contributor

With #107 this should be trivial to implement in any re-frame app, not in the library.

@danielcompton
Copy link
Contributor

@pupeno Thanks for this. A few thoughts:

  1. From the mult docstring:

    Each item is distributed to all taps in parallel and synchronously, i.e. each tap must accept before the next item is distributed. Use buffering/windowing to prevent slow taps from holding up the cult.

    how do we ensure that slow consumers don't stop a re-frame app? If people use a window buffer then they're going to lose events, if they use a buffer then it may fill up eventually and slow everything down.

  2. We're currently thinking about moving away from core.async for re-frame internals. If we merged this, we'd need to support it in the future which could be tricky.

  3. A different route to take would be to allow the developer to register a global callback which is called when dispatch is run. The advantage of this is that it is agnostic to re-frame internals, it's just a function call. This also puts the onus more clearly on the developer to handle dispatches fast.

  4. decouple re-frame and reagent+core.async #107 sounds like another route for the same result.

@pupeno
Copy link
Contributor Author

pupeno commented Oct 16, 2015

Is 3 possible already with re-frame or would that be something that requires implementing?

@danielcompton
Copy link
Contributor

3 would require implementing too.

The main concern I have with this whole idea is that it changes the contract of re-frame from a black box, towards exposing the internals to the programmer. I'm it saying no, but it needs to be considered carefully.

@pupeno
Copy link
Contributor Author

pupeno commented Oct 16, 2015

Yes, I understand. I would personally not worry too much about point 1. I think think tapping the channel should be considered a dangerous thing to do and only do it if you know what you are doing.

Point 2 makes that discussion irrelevant though. Can I ask, what do you plan on using instead of core.async? I found reassuring that re-frame's internals were using the well known and popular core.async.

I think 3 would be the best way to go. I think global handlers and global middlewares could be very useful.

BTW, I'm in slack, as pupeno in the re-frame channel if you want to sync up.

@mike-thompson-day8
Copy link
Contributor

@pupeno where did we get to with this? Do you still need a hook to see events? The router loop no longer uses core.async so it would have to be a callback or some such.

@pupeno
Copy link
Contributor Author

pupeno commented Nov 5, 2015

@mike-thompson-day8 yes, I would still need this. Shall we talk about it sometime next week? Would that be ok? I think chatting about it could be much faster to make progress.

@mike-thompson-day8
Copy link
Contributor

Okay, sure. I have no experience with your usecase.

@danielcompton
Copy link
Contributor

Are you able to just look at the queue in the FSM deftype now that the new version of re-frame is out?

@danielcompton
Copy link
Contributor

I suspect that for your use case, you could just poll the queue state?

@pupeno
Copy link
Contributor Author

pupeno commented Dec 4, 2015

I don't know, I'll check the new code.

J. Pablo Fernández [email protected]
http://pupeno.com
On Dec 4, 2015 19:25, "Daniel Compton" [email protected] wrote:

Are you able to just look at the queue in the FSM deftype now that the
new version of re-frame is out?


Reply to this email directly or view it on GitHub
#118 (comment).

@mike-thompson-day8
Copy link
Contributor

Sorry to be so slow to this @pupeno. But we're on it now. Let's fix this and push a new release.

I'm not very knowledgeable about your usecase (I've never had to do prerendering), so I'm very happy to be guided by you on this.

Have I got this straight: the goal is to have a "hook" which allows code to listen for dispatched events?

If so, the questions which come to mind are:

  • mechanism: callback? core.asyn channel? polling as Daniel suggested?
  • timing - when should the information be "sent". At dispatch time, before handling, after handling?

Happy to do a hangout if that works for you. Like I said, I feel pretty ignorant of the requirements here. I won't be able to see the subtleties in the choices.

@pupeno
Copy link
Contributor Author

pupeno commented Dec 4, 2015

Thank you Mike for looking into this. Yeah, let's do a hangouts and iron
out the details. Will you be around on Sunday?

On 4 December 2015 at 22:36, Mike Thompson [email protected] wrote:

Sorry to be so slow to this @pupeno https://github.com/pupeno. But
we're on it now. Let's fix this and push a new release.

I'm not very knowledgeable about your usecase (I've never had to do
prerendering), so I'm very happy to be guided by you on this.

Have I got this straight: the goal is to have a "hook" which allows code
to listen for dispatched events?

If so, the questions which come to mind are:

  • mechanism: callback? core.asyn channel? polling as Daniel suggested?
  • timing - when should the information to "sent". At dispatch time,
    before handling, after handling?

Happy to do a hangout if that works for you. Like I said, I feel pretty
ignorant of the requirements here. I won't be able to see the subtleties in
the choices.


Reply to this email directly or view it on GitHub
#118 (comment).

J. Pablo Fernández [email protected] (http://pupeno.com)

@mike-thompson-day8
Copy link
Contributor

Yeah, I'm mostly free on Sunday. Sometime in the evening would be easiest for me, but there are other options if necessary.

We'll use this: http://www.timeanddate.com/worldclock/meeting.html

I'm in Sydney. What timezone are you in?

@pupeno
Copy link
Contributor Author

pupeno commented Dec 5, 2015

I'm in London. I'll ping you as soon as I wake up. That should work out
around your evening.

On 5 December 2015 at 03:42, Mike Thompson [email protected] wrote:

Yeah, I'm mostly free on Sunday. Sometime in the evening would be easiest
for me, but there are other options if necessary.

We'll use this: http://www.timeanddate.com/worldclock/meeting.html

I'm in Sydney. What timezone are you in?


Reply to this email directly or view it on GitHub
#118 (comment).

J. Pablo Fernández [email protected] (http://pupeno.com)

@pupeno
Copy link
Contributor Author

pupeno commented Dec 6, 2015

Previously, to sleep for a little while and let re-frame process events, I was doing this:

(cljs.core.async/timeout 300)

More details here: https://github.com/carouselapps/prerenderer/blob/master/src/cljs/prerenderer/re_frame.cljs#L17

What would be the appropriate method with the new queue system? That one doesn't seem to work.

mike-thompson-day8 added a commit that referenced this pull request Dec 6, 2015
Call the appropriate add-post-event-callback. For #118.
@mike-thompson-day8
Copy link
Contributor

I don't think that re-frame could cause core.async/timeout to stop working. In fact, re-frame now doesn't even use core.async at all. There has to be something else going on?

re-frame uses alternative async functions here:
https://github.com/Day8/re-frame/blob/develop/src/re_frame/router.cljs#L62-L63
If they aren't working under Node, could that somehow be triggering this?

@pupeno
Copy link
Contributor Author

pupeno commented Dec 7, 2015

Yes, the timeout is pausing execution for that period of time, but no events are being processed. After my go-loop is done, then all events are processed (and callbacks called)

@mike-thompson-day8
Copy link
Contributor

@pupeno I have prepared version v0.6.0 for release (currently sitting in the develop branch).

When you have satisfied yourself that the new API ticks the boxes, ping me and we'll officially release this version.

@pupeno
Copy link
Contributor Author

pupeno commented Dec 13, 2015

I just released Prerenderer 0.2.0 using the new API: https://carouselapps.com/2015/12/13/prerenderer-0-2-0-released/

Feel free to close this issue whenever you want. I'm satisfied with the current state. Thank you @mike-thompson-day8

@mike-thompson-day8
Copy link
Contributor

Great news. Thanks @pupeno.

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

Successfully merging this pull request may close these issues.

4 participants