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

Fix stream() support for chunked event streams #211

Closed
wants to merge 5 commits into from
Closed

Conversation

aron
Copy link
Contributor

@aron aron commented Mar 5, 2024

There's a subtle bug in the current Stream implementation where if the server emits lines in several chunks then we get "null" events in the async iterator.

For example:

event: output
id: 1
data: hello
-- chunk 2 --
data: world
-- chunk 3

event: done
id: 2
data: {}

Will result in:

SSE { id: 1, event: "output", data: "hello" }
SSE { id: null, event: null, data: "world" }
SSE { id: 2, event: "done", data: "{}" }

This PR fixes this, plus a few bugs in the Stream implementation:

  • We now use the fetch() implementation provided to the Replicate constructor rather than the global.
  • We raise an error if the stream endpoint does not respond with a 2xx status code.
  • We now ensure that the decode() function in the Stream class processes full lines by retaining the last line of the current chunk and appending it to the start of the next chunk before splitting on newlines.
  • Fixes the type definition for validateWebhook which returns a Promise<boolean> but is typed to return boolean.

@aron aron requested a review from mattt March 5, 2024 13:09
expect(await iterator.next()).toEqual({
done: false,
value: { event: "done", id: "EVENT_2", data: "{}" },
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug, rather than a feature because it means that we always need to check event.event === "output" before accessing event.data. The toString function kind of hides this but it doesn't work with JSON.stringify().

I think actually we want to return the done event from the iterator rather than yielding it. Then the done iterator will look like:

expect(await iterator.next()).toEqual({ done: true, value: { event: "done", ... } });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable to me. Any reason not to make that change to return for done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's technically a breaking change, but unclear how much of an issue it would cause. There's nothing you can do with it except listen for it and ignore it.

@aron
Copy link
Contributor Author

aron commented Mar 5, 2024

@mattt can we punt on the event: done piece for the moment while I give it some thought and get the bug fix in?

@aron
Copy link
Contributor Author

aron commented Mar 7, 2024

Closing in favor of #214

@aron aron closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants