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: represent field masks as string[] #525

Merged
merged 5 commits into from
Mar 7, 2022

Conversation

boukeversteegh
Copy link
Collaborator

See my comment at #510

Represent field masks as string[], instead of FieldMask:

Protobuf

syntax = "proto3";

import "google/protobuf/field_mask.proto";

message FieldMaskMessage {
  google.protobuf.FieldMask field_mask = 1;
}

Typescript

export interface FieldMaskMessage {
  fieldMask: string[] | undefined;
}

Changes

  • Move canonical conversion logic from the parent message to FieldMask:
    • parent.toJSON delegates to FieldMask.toJSON(FieldMask.wrap(value))
    • parent.fromJSON delegates to FieldMask.unwrap(FieldMask.fromJSON(value))
  • FieldMask.toJSON converts to canonical JSON directly, so the canonical representation can be used without a parent message
  • FieldMask.fromJSON can accept both canonical form and a one-to-one JSON mapping of the underlying protobuf object {paths: ['foo', 'bar']} for servers that don't serve canonical forms.

Notes

  • This change does stretch the concept of wrap/unwrap beyond its original intention (converting wrapper types). We can consider changing these names. For example: fromNativeType and toNativeType. I think this naming would work for all current cases and may be more clear, but it would be a breaking change.
  • This is technically a breaking change, as the interface for messages with FieldMasks is now different. Not sure if it warrants increasing the major version, as we've only added FieldMask support very recently.

@boukeversteegh
Copy link
Collaborator Author

boukeversteegh commented Feb 28, 2022

The package can now also be installed directly from github, to test PRs (thanks to adding the prepare script).

Install directly from branch with npm

npm install ts-proto 'github:stephenh/ts-proto#feature/field-mask-string-arrays'

Updating to specific commit

npm install ts-proto 'github:stephenh/ts-proto#7a4990c238cdcc8cc3ebe66a6a17cbd446f0e20d'

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.

Thanks for tackling this @boukeversteegh ! I've not yet done enough with the wrap / unwrap approach to 100% internalize it, so this really helped me get how it works.

I had one question about nudging around how we generate FieldMask.fromJSON but feel free to treat that as a musing/todo and just merge as-is. Thanks!

src/main.ts Outdated Show resolved Hide resolved
@@ -1112,6 +1114,16 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
messageDesc.field.filter(isWithinOneOf).filter((field) => field.oneofIndex === oneofIndex)
);

const canonicalFromJson: { [key: string]: { [field: string]: (from: string) => Code } } = {
['google.protobuf.FieldMask']: {
paths: (from: string) => code`typeof(${from}) === 'string'
Copy link
Owner

Choose a reason for hiding this comment

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

I really like the map approach; that's nudging us towards a more configuration-driven vs. the current version if-driven approach.

Just wondering, I wonder we're lucky here that the JSON presentation of FieldMask is a single string value that happens to deserialize into the paths field.

I.e. maybe the key of this map should be "just FieldMask-the type" and we should look in the map, and let it just return the whole fromJson body?

Basically more how like toJSON ended up being handled (although again I definitely like the map-driven approach)...

Wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I did consider doing just that. I went this way to make sure any follow up logic will also applied to fieldmasks. Admittedly, the only logic right now is the fact that we return a single JSON object (instead of constructing one field by field).

It's true that this approach only works exactly in this case, and might not work for other types (like Timestamp?).
If you feel returning the object immediately is more future proof or straightforward, I'm fine to change it.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah cool, I'm good with the current approach!

@stephenh
Copy link
Owner

stephenh commented Mar 4, 2022

🤯

npm install ts-proto 'github:stephenh/ts-proto#feature/field-mask-string-arrays'

I didn't know that was a thing! Neat!

@stephenh
Copy link
Owner

stephenh commented Mar 4, 2022

This change does stretch the concept of wrap/unwrap beyond its original intention

Ah yeah, I'm good with the current wrap/unwrap terminology being used for technically-not-proto-official-wrapper-types...if anything I think it helped my mental model a little bit.

This is technically a breaking change, as the interface for messages with FieldMasks is now
different. Not sure if it warrants increasing the major version, as we've only added
FieldMask support very recently.

Yeah, I'm good with this being a non-breaking change. I'm reserving 2.x for "big" changes like refactoring the config args/etc (no idea when/if that will happen, but basically anything else "not as big as that", I've been doing as major version bumps).

@boukeversteegh boukeversteegh merged commit 903b216 into main Mar 7, 2022
@boukeversteegh boukeversteegh deleted the feature/field-mask-string-arrays branch March 7, 2022 10:05
stephenh pushed a commit that referenced this pull request Mar 7, 2022
# [1.108.0](v1.107.0...v1.108.0) (2022-03-07)

### Features

* represent field masks as `string[]` ([#525](#525)) ([903b216](903b216))
@stephenh
Copy link
Owner

stephenh commented Mar 7, 2022

🎉 This PR is included in version 1.108.0 🎉

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.

None yet

2 participants