sysid / sse-starlette

BSD 3-Clause "New" or "Revised" License
504 stars 35 forks source link

EventSourceResponse should accept AsyncIterable[ServerSentEvent]? #80

Closed peku33 closed 7 months ago

peku33 commented 7 months ago

Hi,

From version 1.8.1 EventSourceResponse constructor stopped accepting AsyncIterable[ServerSentEvent] in content field. It looks like to code inside is compatible - ensure_bytes converts ServerSentEvent to bytes, but typing does not allow it. ServerSentEvent is now exposed only in ping_message_factory type.

sysid commented 7 months ago

@peku33 thanks for reporting this. Typing in 1.8.1. has been (re-)aligned with original EventSourceResponse(Response) from starlette.

Content = typing.Union[str, bytes]
SyncContentStream = typing.Iterator[Content]
AsyncContentStream = typing.AsyncIterable[Content]
ContentStream = typing.Union[AsyncContentStream, SyncContentStream]

Can you maybe provide concrete example where/how you experience the problem? I am willing to fix it.

peku33 commented 7 months ago

Previously I was using it like this:

import json
from asyncio import sleep
from collections.abc import AsyncIterator

from fastapi import APIRouter
from sse_starlette import EventSourceResponse, ServerSentEvent

api_router = APIRouter()

@api_router.get("/", response_class=EventSourceResponse)
async def subscribe() -> EventSourceResponse:
    async def generate() -> AsyncIterator[ServerSentEvent]:
        ordinal = 0
        while True:
            ordinal += 1
            yield ServerSentEvent(
                event="event",
                data=json.dumps({"ordinal": ordinal}),
            )
            await sleep(1.0)

    return EventSourceResponse(
        generate(),
    )

ServerSentEvent objects were converted to bytes with ensure_bytes method called for each item in body stream here: https://github.com/sysid/sse-starlette/blob/main/sse_starlette/sse.py#L240

I believe that ContentStream (realiased in this module?) should also contain AsyncIterable[ServerSentEvent] | Iterable[ServerSentEvent] and possibly other types, as they are accepted by ensure_bytes(data: Union[bytes, dict, ServerSentEvent, Any], sep: str)

sysid commented 7 months ago

Thank you @peku33 , this is helpful! Will attend to it when I find time asap.

vmlopezr commented 7 months ago

@peku33 Thanks for pointing this out! I just hit this issue as well. In my case we pass a dict since ensure_bytes handles it.

For now i just had to throw in the good old # type: ignore on 1.8.1