-
Notifications
You must be signed in to change notification settings - Fork 340
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
feat: delimitedMethods
option
#460
base: main
Are you sure you want to change the base?
Conversation
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.
@AlaaZorkane thanks for the PR!
I had a few questions, and then also if you could make a new integration-tests/delimited-methods
directory with a minimal proto and a test that round trips like:
const foo1 = FooMessage.fromPartial({ ... });
const foo2 = FooMessage.decodeDelimited(FooMessage.encodeDelimited(foo1));
expect(foo2).toEqual(foo1);
That'd be great. Thanks!
src/main.ts
Outdated
input: ${Reader} | Uint8Array, | ||
): ${fullName} { | ||
const reader = input instanceof ${Reader} ? input : new ${Reader}(input); | ||
let end = reader.uint32(); |
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.
Instead of repeating the entire contents of decode
, could this be something like:
const reader = ...
let end = reader.uint32();
return decode(reader, end);
I.e. decode
already accepts an optional length
argument, which I think? is the only thing that has changed here?
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.
You're right! Since delimitedMethods
depends on outputEncodeMethods
we can definitely reuse that logic.
Also @AlaaZorkane , I'm just curious, I've not personally used delimited messages before, but I get the concept and was reading the docs: https://developers.google.com/protocol-buffers/docs/techniques#streaming One thing I'm curious about is, if there are multiple messages in a stream coming in, how do we know which message is whic? Like if a stream of I.e. I kinda assumed that delimited messages would have to encode both the length of the next message, as well as the type of the message, using some sort of schema registry / message id type setup... Or is the assumption that the caller of |
Hey @stephenh - I added the integration tests and the reusing refactor! Would appreciate if you can review them please 🙏 Also, about streaming multiple messages, I think implementations can vary from application to another? And it's mostly a higher level topic that won't be included and/or abstracted in decoding/encoding primitives (like There isn't really a set of best-practices on how to deal with the case of a stream of different types of messages AFAIK, and the assumption I made when I read the P.S: One way I could do what you mentioned is communicate the specific order of messages pre-streaming (?) EDIT: Hey @stephenh just letting you know that I added a custom implementation test for streaming different types of messages via:
Fair to mention that this is one implementation of many and might vary from app to another. |
}, | ||
|
||
encodeDelimited(message: Simple, writer: Writer = Writer.create()): Writer { | ||
return this.encode(message, writer).ldelim(); |
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'm looking at some of the code we create today and it does this:
Nested_InnerMessage.encode(message.message, writer.uint32(18).fork()).ldelim();
And the docs for ldelim
say "Resets to the last state and appends the fork state's current write length as a varint followed by its operations."
This makes me think we need to ensure a fork exists, and change this to be:
return this.encode(message, writer.fork()).ldelim();
Does that seem right?
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.
You're right! It seems like it's the case in the protobuf-js
implementation here. Thanks!
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 have a feeling that this should have been caught by a test. Would it be possible to change the tests to write and read 2 messages instead of 1? Then we will really test the main use-case of this feature @AlaaZorkane
it('decodes length-delimited messages', () => { | ||
const encoded = Simple.encodeDelimited(message).finish(); | ||
const decoded = Simple.decodeDelimited(encoded); | ||
|
||
expect(decoded).toEqual(message); | ||
}); |
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.
How about changing this test (or adding one) to encode and decode 2 messages?
It will give us more confidence that the main use case is working, and also demonstrate how to use the feature in practice.
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.
Definitely! Just added streaming tests via 0ec5e53, please let me know what do you think. Thanks!
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.
One small thing about message
vs. _
but otherwise looks great!
src/main.ts
Outdated
// create the basic function declaration | ||
chunks.push(code` | ||
encodeDelimited( | ||
${messageDesc.field.length > 0 ? 'message' : '_'}: ${fullName}, |
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.
Really minor, but I think this message || _
variable name was for encode
whose for
loop would actually never use message
if there are no fields. For encodeDelimited
, the function body always uses message
, so this can be just message: ${fullName}
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.
Removed in b0f5e38, thanks!
src/main.ts
Outdated
chunks.push(code`const message = this.decode(reader, length);`); | ||
chunks.push(code`return message;`); |
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.
Maybe just return this.decode(...)
?
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.
Yep, definitely!
- removed unused message condition - return decoding directly
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.
Nice tests added! Looks good to me 👍
@AlaaZorkane looks great! Looks like just the build is failing, maybe b/c some output needs updated after getting rebased: https://github.com/stephenh/ts-proto/runs/4766206207?check_suite_focus=true |
Hey @stephenh, sorry can you walk me through it? I already ran the codegen commands and updated the P.S: I took a look at the workflow logs and I am not sure I completely got what the diff is about ^^' |
@AlaaZorkane sure, np; I pulled your PR down locally, and AFAICT just rebasing on main and then re-running codegen for your new I'd push my change up to this PR just to be sure, but I can't push to your Thanks! |
cdf2835
to
7017d4c
Compare
Related issue(s)
#233
This PR adds the following:
encodeDelimited
generatordecodeDelimited
generatordelimitedMethods
option to generate above methodsdelimiatedMethods
usage documentationNotes
I tried to find an API where I don't have to copy paste redundant generation logic but I failed to make it backward-compatible, per example, one of the approaches is to have an option instead of having specific functions like this:
But this would appose these problems:
writer
ornull
explicitlyI also couldn't find a way to not have redundant code for adding cases for each incoming field due to my unfamiliarity with the codebase.431c595Useful links
Protobuf streaming: https://developers.google.com/protocol-buffers/docs/techniques#streaming
Protobuf-js implementations: encodeDelimited and decodeDelimited
Workarounds
This is what I currently do as a workaround, very verbose and loosely typed: