-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
proto3 packed repeated fields #432
Comments
Dear decodeIO: Thank you for your attention. In short, proto3 in C++ seems to treat "repeated double =1" as "[packed = true]" for default, and JS seems to treat as "[packed = false]". Here is a comment for a solution: always put "[packed=true/false]" explicitly. Besides my comment, I really appreciate your amazing/official standard work! Thanks! |
Explicitly putting [packed=true/false] is not valid syntax in proto3. |
This is interesting... |
from https://developers.google.com/protocol-buffers/docs/encoding#packed
|
Could anyone point me to the place in the code where the parsing of "repeated" field type happens? Would love to fork it with just a quick-and-dirty patch for always setting repeated fields as packed. Thanks in advance, would help a lot! |
+1 this is a huge issue for proto3 compatibility @rastapasta I patched this myself in BuilderPrototype.resolveAll -- add this add the end of the function: if (this.ptr.repeated && (this.ptr.type.wireType === 0 || this.ptr.type.wireType === 1 || this.ptr.type.wireType === 5)) this.ptr.options.packed = true; If nothing happens on this issue I'll submit a PR with a slightly cleaner patch. |
PR submitted. I pushed a fixed version of the library to npm as |
@laverdet Usually it is easier to use a git url for a temporary package since npm packages are immutable and forever :( |
Closing this for now. Feel free to send a pull request if this is still a requirement. |
I am having this issue with protobufjs 5.0.1 as well (cannot move to 6.x.x ATM). If we sent a PR like this one, would you consider adding it? |
Never mind. I'll add |
protobuf.js currently does not handle proto3 repeated fields properly.
In proto3, all repeated fields are packed by default but failing to designate a repeated field as [packed=true] causes protobuf.js to incorrectly raise an error:
protobuf.js:3679 Uncaught Error: Illegal wire type for field Message.Field .Holoscanner.Proto.Mesh.triangles: 2 (0 expected)
Adding [packed=true] fixes this issue but this is not valid syntax is proto3.
Specifically:
Causes protobuf.js to raise the iIlegal wire type error while it works fine in other protobuf clients.
This works since protobuf.js correctly treats triangles as a packed field.
The text was updated successfully, but these errors were encountered: