Server-Sent Event support in both modes #21
Conversation
| id: string option; | ||
| data: string option; | ||
| retry: int option; | ||
| empties: string list; (* Lines without a ':' *) |
There was a problem hiding this comment.
I think we should just ignore these at parsing time.
There was a problem hiding this comment.
You mean the empties? While the standard seems to suggest they be comments, they could technically be used to convey application-level information so I was reticent to ignore them outright at this level.
There was a problem hiding this comment.
they're comments, an application using them is wrong :)
| | Frame of sse_frame | ||
| | End_of_stream | ||
|
|
||
| type sse_callback = sse_state -> bool |
There was a problem hiding this comment.
A docstring indicating what the boolean means would be useful :)
There was a problem hiding this comment.
While I agree, and again I'm letting this go, but sometimes we're prey to Fortune 500 enterprises dictating standards breaches that we're forced to implement. I have already had a case with my very first ANSI X12 integration last year. (A field was clearly specified as being fractional to represent percentages, but sure enough I had to make a custom version of that field type for one trading partner which multiplied it by 100…)
| let sq = String.to_seq s in | ||
| match Seq.find_index (fun c -> c = ':') sq with |
There was a problem hiding this comment.
there really is no need to bring Seq in here at all :). List.exists will be faster and simpler, or even in this case List.mem ':' s is enough.
edit: even better here, sorry, is String.index! Way faster.
| let len = Buffer.length body in | ||
| let bf_seq = Buffer.to_seq body in | ||
| (* Search for some complete line *) | ||
| match Seq.find_index (fun c -> c = '\n') bf_seq with |
There was a problem hiding this comment.
same thing, String.index/String.index_from, this kind of stuff.
| | Some pivot -> | ||
| (* Extract line except ending LF *) | ||
| let bf_line = Bytes.create pivot in | ||
| Buffer.blit body 0 bf_line 0 pivot; |
There was a problem hiding this comment.
this is probably better replaced with Buffer.sub
| Buffer.reset body; | ||
| Buffer.add_bytes body bf_after; |
There was a problem hiding this comment.
sad we have to do that, but yes, Buffer is kind of limited :(
Applied as suggested and we'll go from here. Co-authored-by: Simon Cruanes <simon.cruanes.2007@m4x.org>
|
I'd prefer following the standard and treating them, indeed, as comments
:-)
|
I thought I saw some use them in the wild, but I just double-checked Anthropic and it doesn't use them. Not sure where I saw them then. I still see it as risky to delete them outright but I won't oppose it since we're focused on Anthropic. |
| id: string option; | ||
| data: string option; | ||
| retry: int option; | ||
| empties: string list; (* Lines without a ':' *) |
There was a problem hiding this comment.
still needs to be removed! :)
(afaik the browser API removes these, so it's unlikely anyone actually relies on it)
| opt_iter authmethod ~f:(Curl.set_httpauth self.curl); | ||
| opt_iter username ~f:(Curl.set_username self.curl); | ||
| opt_iter password ~f:(Curl.set_password self.curl); | ||
| Curl.set_nosignal self.curl (_get_no_signal ()); |
| let sse_frame_with_id sse_f v = | ||
| { | ||
| event = !sse_f.event; |
There was a problem hiding this comment.
do the same as sse_frame_with_event, and same for the functions below (use with to simplify code)
|
I haven't touched this PR yet, so if things got added or removed something else is modifying things. (In fact I've been sick all this week, so I definitely didn't touch this.) |
2 participants
You should probably consider this a draft to clean up before merging, as it comes straight from our new dev and this is pretty much his first bit of serious OCaml. I'm attaching the test files to this message, although those are of limited value since they were from our monorepo. They might inspire you though. 😉
ezcurl_sse_tests.tar.gz