fanout / django-eventstream

Server-Sent Events for Django
MIT License
664 stars 88 forks source link

Adding support for Django RestFramework #141

Closed enzofrnt closed 4 months ago

enzofrnt commented 6 months ago

Related to: #140

Hi, I've just upgraded the plugin to fully support the Django Rest Framework.

Modifications include:

The Browsable API is fully usable, as shown here:

Preview Image

image

The system will automatically return an SSE Stream when called by a request header that contains Accept: text/event-stream.

jkarneges commented 6 months ago

Thank you. Will look at this soon.

enzofrnt commented 6 months ago

Nice thanks you !

enzofrnt commented 6 months ago

So ? 😋😅

jkarneges commented 5 months ago

Ok, finally looked at this!

There is a lot here. I'd like to split this up:

  1. Please move the new static assets into the chat-restframework example, to minimize the library's surface area. We could maybe graduate these files into the library later but would need to think about it.
  2. Don't change the path of existing assets, at least not yet, since this could break apps and will require a major version bump.
  3. Be sure any needed CORS stuff will work with django-cors-headers too, since I plan to deprecate our built-in CORS logic in favor of everyone using django-cors-headers.

Also, a question: what is the purpose of messages_types?

enzofrnt commented 5 months ago

Hi @jkarneges,

  1. I can't delete those assets from the module. Without those assets, the browsableAPI that I made will not work. But what do you mean by asset ? Every HTML, JS and CSS files ? The goal of the merge-request is to add full support for Django-restframework deleting those files will delete the support for browsableAPI that a part of Django-restframework. image

  2. I will change that. And make an issue related to this upgrade.

  3. I will verify that and push modification or notify here.

enzofrnt commented 5 months ago

Path fix. Looking for CORS tomorow in france.

enzofrnt commented 5 months ago

Message type is use do define what type of event you want to send. That could be helpful in some case.

enzofrnt commented 5 months ago

Everything for CORS look's fine

Sonictherocketman commented 5 months ago

Would love to see this btw. Any update on the status?

enzofrnt commented 5 months ago

Hi, @jkarneges any news about this ? thx.

jkarneges commented 5 months ago

Message type is use do define what type of event you want to send. That could be helpful in some case.

Can you explain this further? It doesn't look like the field is used anywhere.

jkarneges commented 5 months ago

The goal of the merge-request is to add full support for Django-restframework deleting those files will delete the support for browsableAPI that a part of Django-restframework.

Ah, I am not familiar enough with DRF features. So DRF allows browsing endpoints out of the box? Is this kind of thing always on or something the developer opts-in to maybe in debug mode?

enzofrnt commented 5 months ago

Hi @jkarneges,

that something enable by default in Django RestFramework.

image

Usually it's disable in production.

jkarneges commented 5 months ago

Let's put the css file under a css/ path. I'd even be okay with the new js file going under js/ as well, as long as the old js files aren't moved. Be sure the html file is updated to fetch from the right paths.

Usually it's disable in production.

How can it be disabled?

enzofrnt commented 5 months ago

@jkarneges Done :)

How can it be disabled?

Probably with something like that : https://testdriven.io/tips/9aee5b07-bf7a-475b-91cc-d46e8f5c512a/

Django REST is using renderersin this example to disable the browsableAPI we setthe defaultrenderer on json so no more api renderer that made the browsableAPI, it may need some tricks to work with sse here by adding the new renderer that i made

jkarneges commented 5 months ago

The references to the existing js files need to be corrected in the html template.

Message type is use do define what type of event you want to send. That could be helpful in some case.

Can you explain this further? It doesn't look like the field is used anywhere.

I guess the field is used by the browsable view?

It would be good to have a way to disable the browsable view before merging.

enzofrnt commented 5 months ago

Hi, please do not accept the merge request now, i'm improving some things about the browsable view… and to be able to disable it as we can with original browsable view. Also, @jkarneges can i take a look at the conflict to resolve them ? thx.

enzofrnt commented 5 months ago

I did not add comment or docstring because the rest of the code didn't have comment. Maybe create an issue related to this upgrade ?

enzofrnt commented 5 months ago

It is now possible to disable the browsable view. The message type is related to SSE in general. SSE uses message types to communicate specific events, such as the "keep-alive" message that is used to keep the stream active, and the "error" message to indicate an error. This allows us to send other types of messages if needed, to handle specific scenarios in our application.

enzofrnt commented 5 months ago

Hi, @jkarneges any news ?

enzofrnt commented 4 months ago

🙏🏻

jkarneges commented 4 months ago

Hi @enzofrnt, the branch is still showing a conflict.

enzofrnt commented 4 months ago

Hi, @jkarneges every thing is fix. Thank you.

jkarneges commented 4 months ago

Getting this error when running the example:

  File "/Users/jkarneges/src/fanout/django-eventstream/examples/chat-restframework/../../django_eventstream/views/sseview.py", line 87, in _stream_or_respond
    data = {'channels': ', '.join(channels) or "", 'messages_types': ', '.join(self.messages_types) or "message"}
                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: can only join an iterable
enzofrnt commented 4 months ago

HI, @jkarneges just fixed it ! Finally :)

jkarneges commented 4 months ago

Almost there.

I tried disabling the browser view by removing the renderer like this:

REST_FRAMEWORK = {
    'DEFAULT_RENDERER_CLASSES': [
        'rest_framework.renderers.JSONRenderer',
        #'rest_framework.renderers.BrowsableAPIRenderer',
        'django_eventstream.renderers.SSEEventRenderer',
        #'django_eventstream.renderers.BrowsableAPIEventStreamRenderer'
    ] 
}

But this causes a strange response to be served:

Screenshot 2024-05-22 at 5 26 46 PM
enzofrnt commented 4 months ago

Maybe because the room don't exist.. But not sure i will take a look at this.

enzofrnt commented 4 months ago

Now, every thing may work as needed.

enzofrnt commented 4 months ago

As junior/student dev, I made a lot of mistake. Sorry.

enzofrnt commented 4 months ago

Hi, any news ? : )

jkarneges commented 4 months ago

I may clean some things up before the next release but this looks good! Thank you :)

enzofrnt commented 4 months ago

Thanks you !