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: generate canonical JSON encoding for FieldMasks #510

Merged
merged 2 commits into from
Feb 14, 2022
Merged

fix: generate canonical JSON encoding for FieldMasks #510

merged 2 commits into from
Feb 14, 2022

Conversation

buyology
Copy link
Contributor

@buyology buyology commented Feb 13, 2022

Fixes/updates #497.

The canonical JSON encoding of FieldMasks is a comma-separated string of the FieldMask.paths-array. Generate that JSON mapping for FieldMask-fields.

In #497 @alexeyten suggested that

[…] probably in JS this should be represented as an array of string.

This change does not touch the representation of the FieldMask. I checked other generator libs, e.g. the Go generator, and it did not touch the representation of the FieldMask. It is probably also simpler not do so in order to get the correct wire-encoding "for free".

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.

This change does not touch the representation of the FieldMask.
I checked other generator libs, e.g. the Go generator, and it did
not touch the representation of the FieldMask. It is probably also
simpler not do so in order to get the correct wire-encoding "for free".

Cool, that makes sense. This looks great!

Fwiw I think ts-proto's "idiomatic TypeScript" mission statement would generally lean towards using the string[] representation directly in the message interfaces themselves as well, but it's also definitely np to defer that to a separate PR.

@stephenh stephenh merged commit 0ec4e97 into stephenh:main Feb 14, 2022
stephenh pushed a commit that referenced this pull request Feb 14, 2022
## [1.105.1](v1.105.0...v1.105.1) (2022-02-14)

### Bug Fixes

* generate canonical JSON encoding for FieldMasks ([#510](#510)) ([0ec4e97](0ec4e97))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.105.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@boukeversteegh
Copy link
Collaborator

Awesome! I was just going to use field-masks but refrained after noticing this missing feature. Thanks 👍

For the typescript representation part, we could use the wrap/unwrap approach as we do for Wrapper types.
So it won't be necessary to re-implement the encoding/decoding.

Basically, the FieldMask representation would be kept as-is, but it will be responsible for the JSON coding.

The container messages will:

  • generate json for a fieldmask field like this: FieldMask.ToJson(FieldMask.wrap(message.someFieldMask))
  • and parse json like this: message.someFieldMask = FieldMask.unwrap(FieldMask.FromJson(obj.someFieldMask)).

That way we can keep a 'clean' FieldMask concept, which is a true representation of the Protobuf object, which gives the wire-format out of the box, while allowing for TS-friendly types within the container messages.

Additionally, the logic for the canonical json is isolated in separate methods.

Let me know if your thoughts and if you're interested in implementing this. Otherwise I might look into it if I have some time.

@boukeversteegh
Copy link
Collaborator

hi @buyology , I went ahead to add the above suggestions. Please have a look if this implementation works for you.

See #525

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.

3 participants