-
Notifications
You must be signed in to change notification settings - Fork 4
general improvements #7
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
base: master
Are you sure you want to change the base?
Conversation
rawhat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks for all this work!
src/gramps/websocket.gleam
Outdated
| // Complete standalone frame | ||
| [Complete(frame), ..rest], None -> { | ||
| case frame { | ||
| Data(CompressedTextFrame(data)) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that definitely sounds better to me, i think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)