Skip to content
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: enable unknown fields for descriptor protos #479

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

Vilsol
Copy link
Collaborator

@Vilsol Vilsol commented Jan 17, 2022

Builds upon #473 to add support for options in #437.

I had to also enable --downlevelIteration tsc flag as that is now a requirement due to looping over Uint8Array.

The tests will most likely need to be updated after the package is published.

@stephenh stephenh merged commit 824c996 into stephenh:main Jan 18, 2022
stephenh pushed a commit that referenced this pull request Jan 18, 2022
# [1.102.0](v1.101.0...v1.102.0) (2022-01-18)

### Features

* enable unknown fields for descriptor protos ([#479](#479)) ([824c996](824c996))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.102.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@stephenh
Copy link
Owner

@Vilsol looks great! The ts-proto-descriptors package is still manually published, so I just did that locally, and this should be in npm as version 1.4.0.

Fwiw I might have pinned ts-proto-descriptors to ts-proto that had optional-field behavior that worked with protobuf v2's oneofIndex...

Like at one point (which is probably still true today), how ts-proto handled optional fields meant that (with the ts-proto-descriptors generated by that version of ts-proto) we couldn't tell the difference between oneofIndex=0-because-its-unset and oneofIndex=0-because-its-actually-zero.

I think it was this commit:

3e78405

...more context in this issue #218

...and then it goes back to this one #139 which is basically highlighting that ts-proto does not have the hazzer methods that most protobuf generated messages do (b/c we're "just data"), so we can't apply the behavior of "the data is 0 ... but also hasData is false".

So...you can try bumping ts-proto-descriptors to 1.4.0 and see what happens, but after re-remembering things, I'm 90% sure you're going to hit this limitation. :-/

Wdyt? Any good ideas?

@Vilsol Vilsol deleted the update-protos branch January 18, 2022 14:26
@Vilsol
Copy link
Collaborator Author

Vilsol commented Jan 19, 2022

@stephenh It looks like indeed that causes a lot of issues. One alternative we could have is provide yet another metadata field behind some option flag like definedFields.

All it would do is add the field name to a list (or set) when it was decoded. So this:

case 9:
    message.oneofIndex = reader.int32();
    break;

would become

case 9:
    message._definedFields[tag] = __spread((message._definedFields[tag] || []), ['oneofIndex']);
    message.oneofIndex = reader.int32();
    break;

I see how this is an ugly solution, but I can't think of any other that wouldn't also impact other users of the generator.

@stephenh
Copy link
Owner

@Vilsol yeah... I guess if we have to go the flag route (which I think we do...) we could add a flag that opts into the Object.create behavior that is in the currently-pinned version of ts-proto that ts-proto-descriptors uses.

I.e. it put the defaults into a prototype, so the hazzer for "is oneOfIndex explicit or defaulted" was "if it's in the object's own keys, it was explicitly set".

Personally I really like that behavior, it just sucks that Object.keys does not pull in keys from the prototype chain (I get having an "own" version of it like Object.getOwnkeys or what not, but would be nice if the default Object.keys included the prototype...like that's what the prototype is for imo...), and so it was a large breaking change to ts-proto, and had to be rolled back.

But, complaining about Object.keys aside, I think some sort of usePrototypeForDefaults=true, which is what that reverted PR did, would let us enable it for ts-proto-descriptors and then correctly handle oneofs...

The upshot would also be that then anyone else who really needs hazzer methods could use the same flag...

Wdyt? Do you want to try and resurrect that old PR? I can as well, disclaimer not sure when I'd get to it...

@Vilsol
Copy link
Collaborator Author

Vilsol commented Jan 20, 2022

@stephenh Seems like that is the only way out of this. When you say PR, I assume you meant the issue #139 ?

Either way, I can try implementing such a flag, seems like it would just be the 2 changes to main.ts in this commit, no? 3e78405#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants