Don't encode proto2 optional fields #973
Comments
|
Hi @Demon000 , proto2 should be supported; it was not an initial goal of ts-proto, but I think it basically works at the moment. The biggest flag for proto2 support is Otherwise, yeah, it should work. Let me know if it doesn't, and I can try and point you in the right direction of a fix/PR. Thanks! |
Thanks for the quick answer. The first issue I encountered is that fields marked as The second issue I have is that fields with set default values will not be sent over the wire. I'm not sure how usePrototypeForDefaults helps with these cases, it doesn't seem to. |
Ah, okay, that makes sense. I think the only place I've personally used proto2 optional fields is reading them, and not putting them on the wire. I think we'd need some sort of |
I guess this makes sense...when ts-proto was first written, it was when proto3 didn't have optional fields. Then after a few years, proto3 added support for optional fields, as like oneofs, and so we support that ... but probably not the "quick & simple" approach of proto2 of just "do or do not put the value on the wire". Fwiw personally I wish proto3 would have kept proto2's optional field semantics, instead of asserting for ~4-5 years that "no one needs optional fields", and then just backtracking on it later. 🤷 Apologies for the late replies, I can't always immediately triage ts-proto issues, but try to check-in about once per week or two. I think both of these issues you're seeing are fixable if you're interested in poking around, like the |
|
Sorry, I switched to |
stephenh
changed the title
|
Ah yeah; I won't deny that--ts-proto's "strength" is that its collected a large array of knobs/flags over the years that, in theory, let users adapt the output to whatever their random/esoteric protobuf internal stacks, but yeah the complexity has gone up to support that. 🤷 Not really a problem I'm paid to solve :-) , so happy you found protobuf-es does what you want. |
### Description This PR aims to implement the features necessary to enable proto2 optional fields and support proto2 default values, requested by #973 The following changes are present in this PR: - Code changes to `main.ts` and `types.ts` that implement **optional fields**, as well as [**default values**](https://protobuf.dev/programming-guides/proto2/). - Creates two new options: `disableProto2Optionals` and `disableProto2DefaultValues`. By default these are set to false. - New integration tests for `proto3` and `proto2` syntax types, intended to catch any regressions - Modifications to existing integration tests - this mainly involves: - Updating message interfaces to use optional variables - Updating the createBaseMessage() function to use default values provided by the proto2 syntax. - The following two changes to the `encode` function of a message 1. When checking boolean default values, following the other default checks by checking if not equals instead of equals: ```ts // BEFORE message.truth === true // AFTER message.truth !== false ``` 2. When default checking Long types using the `forceLong=true` option, always check via a `.equals()` instead of `.isZero()`. This could be reverted, however, I find it to be cleaner; regardless of whether there is a default value provided or not this check will always work. ```ts // BEFORE !message.key.isZero() // AFTER !message.key.equals(Long.ZERO) ``` ### Outstanding work In a **subsequent future PRs**, we need to handle the following: - Updating to `ts-proto-descriptors` to correctly render fields as optional. This involves running changing the parameters for `protos/build.sh` to using the build code from `ts-protos`. I tried doing this as part of this PR, but the updated proto descriptors did not work well with ts-proto. Going to re-attempt this at a later date. - Fix the last of the default values: currently all but the Bytes type is handled correctly. I have added a `todo(proto2)` label around the places that need updating. ## Past Discussion Please ignore everything below, as it is out of date. I am keeping it within the PR description so below comments make sense to future readers. ##### Broken TypeScript in `src` > @lukealvoeiro: The code in `src/` is currently broken as a result of the `ts-proto-descriptors` update, which made a lot of the fields typically available in interfaces such as `ProtoFileDescriptor` optional. > > I'm planning on updating all the source code to use the optional syntax (not actually that hard, there are like 60 TS errors which I can probably knock out quickly), and then adding a new option that disables proto2 optional fields (e.g. `disableProto2Optionals=true`). This gives us better maintainability in the long run, brings us in line with proto2 conventions, and improves our parsing of proto files. It also allows customers who are used to ts-proto's existing output for proto2 files to continue to codegen the same TS output via the `disableProto2Optionals` flag. As I mentioned above, I tried do this but didn't get very far down this path. I think in general it makes sense to update `ts-proto-descriptors` to use the optional fields (and default values), however, it was harder than expected to get the updated descriptors working with `ts-proto` ##### File-specific context > @lukealvoeiro: One of the not-so-elegant aspects of this PR is visible in `main.ts`, where I am passing a parameter `isProto3File: boolean` around a lot. Wanted to open up a discussion as to whether it would be helpful to insert file-specific context such as `isProto3File` into the Context object. After we process each file, we could then overwrite this portion of the context with the next file's metadata. I implemented this change, folks can continue adding to the file context whenever appropriate. ### Testing performed - [x] Ran unit tests, changes to files are expected and compile / pass tests - [x] Ran on my own codebase that has proto2 and proto3 files and noticed no issues there either.
|
Fwiw @lukealvoeiro I was looking for any issues tagged with "proto2" to close out, after you landed #1007 ... seems like this one is still valid, i.e. an ask to recognize field values as "matching the optional value", and not encode them. Which is really similar to what proto3 does with its default values, but probably still something we don't do, which is fine imo/not high priority, just noting my current understanding. |
Is this supported and I just can't find the correct flags to use, or is this unsupported?
The text was updated successfully, but these errors were encountered: