guyzmo / event-source-library

Python Event Source Library
GNU General Public License v3.0
35 stars 9 forks source link

integration clarification #17

Closed ghost closed 8 years ago

ghost commented 8 years ago

Existing docs only briefly touch the question of integrating with EventSource without use of external post handler. EventSourceHandler.buffer_event() is advised but more elaborate examples are certainly required: how can we construct EventSourceHandler, how can we pass it to tornado's application and to external code which will call buffer_event().

guyzmo commented 8 years ago

Well, this code has been designed like a binary, and the whole application offered with it is a very simple — and basic use case — example on how to use the lib!

ghost commented 8 years ago

The problem is that given example is way too brief. I've added r"/(.*)/(.*)" route with listener and it doesn't work. The requester in send_json() uses /ping/token as a post url - should there be explicit route added for it with r"/ping/(.*)"? Or is it added implicitly by listener? If latter is the case, than how many routes are added and which ones? And what if application already have multitude of routes? The pattern r"/(.*)/(.*)" is too generic and might interfere with other code paths so it would be nice to move it to r"/sse/(.*)/(.*)" but that again brings up the question of r"/ping/(.*)" - shall send_json() use r"/sse/ping/token" in this case?

guyzmo commented 8 years ago

The pattern r"/(.)/(.)" is too generic and might interfere with other code paths so it would be nice to move it to r"/sse/(.)/(.)" but that again brings up the question of r"/ping/(.*)" - shall send_json() use r"/sse/ping/token" in this case?

Well, I did not think too hard about giving different paths to the tool, as you would prefer to use a webserver as "proxy" to route requests between clients and the service — which should be running as standalone if wsgi still cannot keep long polling connections open… I hope it's not an issue anymore in 2016 ☺. And then, using a simple url rewrite rule, you can place the service's path under any path you want. That being said, what you say should not be hard to change that path as you're suggesting.

The requester in send_json() uses /ping/token as a post url - should there be explicit route added for it with r"/ping/(.*)"? Or is it added implicitly by listener?

Well it's actually the first part of the /.*/.* rule! When you send a post request to /ping/<uuid>, you call the post() method with `(action='ping', target='').

Then you'll see that it checks whether the _connected member contains the target, and that the action is defined in the ACTIONS member of _event_class. And you do define the _event_class member through the initialize() method. And obviously, an event_class is an instance of the Event class ☺ So any of JSONIdEvent, StringIdEvent, JSONEvent, StringEvent… All of which carry the allowed actions to be sent within the ACTIONS member of the Event class. Then the Event class is responsible to do something with the data you sent through the SSE. So you can see that JSONEvent.get_value() and JSONEvent.set_value() are respectively encoding and decoding JSON data, so if you provide ill-formatted JSON, it won't accept it.

So if you want to extend the library, you really want to focus on two things:

guyzmo commented 8 years ago

(N.B.: could you use markdown formatting in your issues posts? So people coming to this repo can easily read your comments?)

ghost commented 8 years ago

Thanks for answer. Not sure why, but replacing send_json() with tornado's tornado.httpclient.AsyncHTTPClient() fixed it for me. The only additional change necessary was url prefix in eventsource-client - I can make it optional and send PR once current PR is merged.

guyzmo commented 8 years ago

please send that other PR, even if it's based on the other PR's code. I'll check them both when I'll have time to give them a look.

ghost commented 8 years ago

Sent, please have a look.