Suggestion: Cleaner Frame API

[leonerd]

The current P:WS:Frame API is somewhat hard to understand. It took me a while of source-reading before I could work out what to do with it.

In very simple cases where you're just dealing with text frames, you can just keep a single $frame object, $frame->append($bytes) from the wire, and then read individual blobs of text with while(defined( my $payload = $frame->next ) ).

However, this gets more complicated the moment you have other frame types - binary, ping/pong, close. While there are the type accessors, is_ping etc, they implicitly apply to the most-recently-returned frame, whose payload was returned by ->next. This makes the Frame object a sortof bizarre hybrid of a buffer of pending frames still under construction and an accessor for the metadata of the current one.

Instead, I would suggest two objects; one a buffer for assembling bytes into frames (and performs the fragment reassembly), which returns ready-assembled frame objects.

my $framebuffer = Protocol::WebSocket::FrameBuffer->new;
$framebuffer->append( $bytes_from_the_wire );

while( my $frame = $framebuffer->next_frame ) {
  say $frame->payload if $frame->is_text;
}

Additionally this would get over the otherwise-impossibility of not knowing whether to call ->next or ->next_bytes, because you don't yet know if the next frame will be a text or binary frame.

---

Failing that (if perhaps it's deemed too difficult by now to change the API style somewhat), could I at least suggest an improvement to the documentation of the opcode and is_* helper methods? It isn't very clear that these methods apply to the most recently ->next'ed payload, as opposed to the one that would be received by it, or anything else.

[vti]

Yeah, I see the problem and agree that it needs to be fixed. Maybe a new FrameBuffer and a new SaneFrame (or smth like that :) should be introduced, so the working code will still be working and new code will use just another objects.

[leonerd]

Actually thinking about it, a FrameBuffer isn't really necessary. Far simpler, at least in IO::Async's case, would be to have a Frame->from_bytes( $bytes ) method that atomically either extracted exactly-one frame from $bytes and returned it, or returned undef. This would consume bytes from the $bytes buffer.

It would then be really easy to use as

on_read => sub {
  my ( $self, $buffref, $eof ) = @_;
  while( my $frame = Protocol::WebSocket::Frame->from_bytes( $$buffref ) ) {
   $self->invoke_event( on_frame => $frame );
  }
  return 0;
};

though that might complicate other event systems, where the user is expected to maintain their own buffer between read/recv calls. Perhaps this is the service the FrameBuffer could provide?

[vti]

There should be definitely an alternative way of extracting a frame without modifying a buffer.

Maybe:

my $frame_buffer = Protocol::WebSocket::Frame->new();
$frame_buffer->append($buffer);
while (my $frame = $frame_buffer->next_frame){
}

So in addition to the next and next_bytes a new next_frame method is implemented which does exactly what you propose. And it stays backwards compatible.

[vti]

But this does not eliminate the idea of introducing a new FrameBuffer class for the new applications of course.