Repository navigation
general improvements - #7
vshakitskiy wants to merge 5 commits into
Conversation
rawhat
left a comment
There was a problem hiding this comment.
awesome, thanks for all this work!
| // Complete standalone frame | ||
| [Complete(frame), ..rest], None -> { | ||
| case frame { | ||
| Data(CompressedTextFrame(data)) -> { |
There was a problem hiding this comment.
so this is an interesting one... and is making me think a bit more about the types we have here
with the addition of these to DataFrame and the panics above, i wonder if we should have some intermediate type that's only used within the contexts of these parsing functions. if this type is on the public DataFrame type, now any consumer matching on it has to handle this, when it's not really valid outside of this library's handling of the bits
even if it's just a wrapper only used in here around the compress-able frames, that seems maybe fairly innocuous? does that make sense?
There was a problem hiding this comment.
I think we can some InternalDataFrame type that will be used only within parsing functions, and have public DataFrame with only TextFrame and BinaryFrame. What do you think?
There was a problem hiding this comment.
that definitely sounds better to me, i think!
There was a problem hiding this comment.
after playing with the code a little, it seems like it would be not that easy to implement my initial proposal with InternalDataFrame type. With current public functions & types there will be constant mappings from internal types to public ones, the codebase will become very overcomplicated. There is few options that I can see here: we can figure out different solution that removes CompressedTextFrame & CompressedBinaryFrame, but right now I have no idea how to do that 😅... Or we can have these types public for now and later in next releases hide that types from consumer
There was a problem hiding this comment.
I think using Gleam's opaque for DataFrame could also be a clean way forward. We keep all four variants in the type for internal parsing/aggregation, but make it opaque to hide constructors from users. Then, add public constructors just for TextFrame and BinaryFrame, and a match_data_frame function that takes callbacks for text/binary payloads and includes a bool is_compressed flag. This lets users handle incomplete compressed frames without direct variant matching, avoids new internal types or mapping boilerplate, and ensures aggregation always delivers decompressed public variants. What do you think about this one?
|
I implemented my proposed idea with If you think this is not the right approach, i can revert the |
After running Autobahn tests on ewe WebSocket implementation and successfully passing all the cases, here are some general improvements:
NeedMoreDatainstead ofInvalidFrameaggregate-framesto accumulate payloads in a list and concatenate once at the end, rather than incremential bit array appendsapply_inflateto correctly callcompression.inflateinstead ofcompression.deflate(apply_inflatecalling wrong function #6)decode_many_frames_resultfunction with proper error propagation (decided to not touchdecode_many_frames)