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

fix: fix codegen for maps with wrapper value type #370

Merged
merged 7 commits into from
Oct 28, 2021

Conversation

aikoven
Copy link
Collaborator

@aikoven aikoven commented Oct 23, 2021

Before this fix a map<string, google.protobuf.StringValue> would generate invalid TypeScript code like

message.mapOfStringValues[key] = string | undefined.fromJSON(value);

Fixes #182

@aikoven
Copy link
Collaborator Author

aikoven commented Oct 23, 2021

I had to push the google/protobuf/timestamp.ts to make CI happy. It has a slight difference to the one in the repo, probably due to a different version of protoc.

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

integration/simple/simple-test.ts Outdated Show resolved Hide resolved
integration/simple/simple-test.ts Outdated Show resolved Hide resolved
integration/simple/simple-test.ts Outdated Show resolved Hide resolved
integration/simple/simple.ts Show resolved Hide resolved
Copy link
Collaborator Author

@aikoven aikoven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review @stephenh.

I've updated the tests, though there are some questions regarding undefined values behavior.

I'm not sure whether we should preserve them in generated objects. In the wire format, we can distinguish between a missing map entry and an empty entry value. But in the generated code if we erase the undefined values, we can't distinguish between the two.

On the other hand, we already drop map entries with empty values if that map has a message value type.

Anyway, maps with wrapper value types seem like a marginal case to me, and I only need them to work to support third-party protobufs. So I'd be happy even if we lose some strictness.

integration/simple/simple-test.ts Show resolved Hide resolved
integration/simple/simple-test.ts Show resolved Hide resolved
integration/simple/simple-test.ts Show resolved Hide resolved
Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aikoven cool, this looks good to me. I think you're changes look good, and yeah I'm good with loosing some strictness too, in terms of being pragmatic. I mostly wanted tests that at least documented what we do today, so that we know when/if it changes, which I think this current round does pretty well.

Feel free to continuing making pragmatic decisions and merge whenever the build passes. :-) Thanks!

integration/simple/simple-test.ts Show resolved Hide resolved
integration/simple/simple-test.ts Show resolved Hide resolved
integration/simple/simple-test.ts Show resolved Hide resolved
@aikoven
Copy link
Collaborator Author

aikoven commented Oct 28, 2021

I ended up skipping entries with undefined values when encoding these maps. This made pbjs happy as well.

Feel free to continuing making pragmatic decisions and merge whenever the build passes. :-) Thanks!

All tests pass now and I think that the PR is ready to merge. But I don't have the merge button:

image

@stephenh
Copy link
Owner

Ah shoot! I thought I'd invited you as a collaborator already, but actually had not; just sent an invite.

@aikoven
Copy link
Collaborator Author

aikoven commented Oct 28, 2021

Great, thanks!

@aikoven aikoven merged commit dd2481d into stephenh:main Oct 28, 2021
@aikoven aikoven deleted the fix-map-with-wrapper-value branch October 28, 2021 18:27
stephenh pushed a commit that referenced this pull request Oct 28, 2021
## [1.83.3](v1.83.2...v1.83.3) (2021-10-28)

### Bug Fixes

* fix codegen for maps with wrapper value type ([#370](#370)) ([dd2481d](dd2481d))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.83.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Map with value as google wrapper type produces invalid fromJSON and fromPartial code
2 participants