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

add well known extensions #93

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jonahsmith
Copy link

@jonahsmith jonahsmith commented Feb 27, 2021

This PR creates a mechanism for including extended methods for well known types, and adds those extensions for the Timestamp type as well as ListValue (in Struct), just to prove out the concept beyond the immediate need. It does so by echoing the logic in google-protobuf, which should provide exact logical parity:

Note that the embeds are copied over from DefinitelyTyped.

Happy to write out the rest of the embeds generated by the Google compiler, but let me know if this is a reasonable approach.

Addresses #92

@@ -17,6 +17,7 @@ export namespace ProtoMsgTsdFormatter {
messages: MessageFormatter.IMessageModel[];
extensions: ExtensionFormatter.IExtensionModel[];
enums: EnumFormatter.IEnumModel[];
wellKnownDeclarations: string[];
Copy link
Owner

Choose a reason for hiding this comment

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

We need Dedicated Types here rather than string type

Copy link
Author

Choose a reason for hiding this comment

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

Okay got it, thanks. Do you have a suggestion on what those would be? I was modeling this after the imports field, since they are roughly equivalent (static strings that simply need to be inserted at the bottom of the file), but this could also be aliased like type WellKnownDeclaration = string; if that's preferable.

"Timestamp": [
"toDate(): Date;",
"fromDate(date: Date): null;",
"static fromDate(date: Date): Timestamp;",
Copy link
Owner

Choose a reason for hiding this comment

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

I think we need move those functions into templates

Copy link
Author

Choose a reason for hiding this comment

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

Could you advise on the best structure for that? Are you imagining partials for each of the messages within each of the well known types? I could imagine for example:

template
  partial
    wellKnown
      timestamp
        declarations.hbs
        extensions
          [messageName].hbs

I was primarily concerned about how these would be passed conditionally into the messages in a way that is not too obscenely complicated.

@@ -16,3 +16,6 @@
{{#each enums}}
{{{render 'partial/enum' this}}}
{{/each}}
{{#each wellKnownDeclarations}}
{{{this}}}
Copy link
Owner

Choose a reason for hiding this comment

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

I think we need partial/xxx sub templates for well known types

Copy link
Author

Choose a reason for hiding this comment

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

Cool, I'll wait to hear back on whether you prefer a sub template setup from elsewhere before implementing just so I know we're on the same page.

@@ -58,13 +60,20 @@ export namespace ProtoMsgTsdFormatter {
return Utility.formatOccupiedName(str);
});

if (fileName in WellKnownExtensionsMap) {
WellKnownExtensionsMap[fileName]?.declarations?.forEach(declaration => {
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't get here. fileName here is the proto file user provided, like the examples/book.proto. It will never match the WellKnownExtensionsMap.

Copy link
Author

@jonahsmith jonahsmith Mar 7, 2021

Choose a reason for hiding this comment

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

Yes, that's expected! In that scenario, the Google generator will also not match, and therefore those functions will not be present. (The current example does not actually generate descriptors for any well known types.)

Those fields are strictly available when you're translating a well known type with that file path — see my original PR description for links to where/when this happens in Google protoc. I will add an example here to illustrate, though.

@@ -45,7 +47,7 @@ export namespace ProtoMsgTsdFormatter {
});

descriptor.getMessageTypeList().forEach((enumType) => {
messages.push(MessageFormatter.format(fileName, exportMap, enumType, "", descriptor));
messages.push(MessageFormatter.format(fileName, exportMap, enumType, "", descriptor, WellKnownExtensionsMap[fileName]?.extensions));
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, file name is user provided like examples/book.proto. It won't be in the WellKnownExtensionsMap.

Copy link
Author

Choose a reason for hiding this comment

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

See comment here

@agreatfool
Copy link
Owner

Could you please share me some use cases of google/protobuf/timestamp.proto?

I added the

message TimestampType {
    google.protobuf.Timestamp time = 1;
}

in the examples/proto/book.proto file, and found generated examples/src/grpcjs/proto/book_pb.js added

if (jspb.Message.GENERATE_TO_OBJECT) {
/**
 * Creates an object representation of this proto.
 * Field names that are reserved in JavaScript and will be renamed to pb_name.
 * Optional fields that are not set will be set to undefined.
 * To access a reserved field use, foo.pb_<name>, eg, foo.pb_default.
 * For the list of reserved names please see:
 *     net/proto2/compiler/js/internal/generator.cc#kKeyword.
 * @param {boolean=} opt_includeInstance Deprecated. whether to include the
 *     JSPB instance for transitional soy proto support:
 *     http://goto/soy-param-migration
 * @return {!Object}
 */
proto.com.book.TimestampType.prototype.toObject = function(opt_includeInstance) {
  return proto.com.book.TimestampType.toObject(opt_includeInstance, this);
};


/**
 * Static version of the {@see toObject} method.
 * @param {boolean|undefined} includeInstance Deprecated. Whether to include
 *     the JSPB instance for transitional soy proto support:
 *     http://goto/soy-param-migration
 * @param {!proto.com.book.TimestampType} msg The msg instance to transform.
 * @return {!Object}
 * @suppress {unusedLocalVariables} f is only used for nested messages
 */
proto.com.book.TimestampType.toObject = function(includeInstance, msg) {
  var f, obj = {
    time: (f = msg.getTime()) && google_protobuf_timestamp_pb.Timestamp.toObject(includeInstance, f)
  };

  if (includeInstance) {
    obj.$jspbMessageInstance = msg;
  }
  return obj;
};
}


/**
 * Deserializes binary data (in protobuf wire format).
 * @param {jspb.ByteSource} bytes The bytes to deserialize.
 * @return {!proto.com.book.TimestampType}
 */
proto.com.book.TimestampType.deserializeBinary = function(bytes) {
  var reader = new jspb.BinaryReader(bytes);
  var msg = new proto.com.book.TimestampType;
  return proto.com.book.TimestampType.deserializeBinaryFromReader(msg, reader);
};


/**
 * Deserializes binary data (in protobuf wire format) from the
 * given reader into the given message object.
 * @param {!proto.com.book.TimestampType} msg The message object to deserialize into.
 * @param {!jspb.BinaryReader} reader The BinaryReader to use.
 * @return {!proto.com.book.TimestampType}
 */
proto.com.book.TimestampType.deserializeBinaryFromReader = function(msg, reader) {
  while (reader.nextField()) {
    if (reader.isEndGroup()) {
      break;
    }
    var field = reader.getFieldNumber();
    switch (field) {
    case 1:
      var value = new google_protobuf_timestamp_pb.Timestamp;
      reader.readMessage(value,google_protobuf_timestamp_pb.Timestamp.deserializeBinaryFromReader);
      msg.setTime(value);
      break;
    default:
      reader.skipField();
      break;
    }
  }
  return msg;
};


/**
 * Serializes the message to binary data (in protobuf wire format).
 * @return {!Uint8Array}
 */
proto.com.book.TimestampType.prototype.serializeBinary = function() {
  var writer = new jspb.BinaryWriter();
  proto.com.book.TimestampType.serializeBinaryToWriter(this, writer);
  return writer.getResultBuffer();
};


/**
 * Serializes the given message to binary data (in protobuf wire
 * format), writing to the given BinaryWriter.
 * @param {!proto.com.book.TimestampType} message
 * @param {!jspb.BinaryWriter} writer
 * @suppress {unusedLocalVariables} f is only used for nested messages
 */
proto.com.book.TimestampType.serializeBinaryToWriter = function(message, writer) {
  var f = undefined;
  f = message.getTime();
  if (f != null) {
    writer.writeMessage(
      1,
      f,
      google_protobuf_timestamp_pb.Timestamp.serializeBinaryToWriter
    );
  }
};


/**
 * optional google.protobuf.Timestamp time = 1;
 * @return {?proto.google.protobuf.Timestamp}
 */
proto.com.book.TimestampType.prototype.getTime = function() {
  return /** @type{?proto.google.protobuf.Timestamp} */ (
    jspb.Message.getWrapperField(this, google_protobuf_timestamp_pb.Timestamp, 1));
};


/**
 * @param {?proto.google.protobuf.Timestamp|undefined} value
 * @return {!proto.com.book.TimestampType} returns this
*/
proto.com.book.TimestampType.prototype.setTime = function(value) {
  return jspb.Message.setWrapperField(this, 1, value);
};


/**
 * Clears the message field making it undefined.
 * @return {!proto.com.book.TimestampType} returns this
 */
proto.com.book.TimestampType.prototype.clearTime = function() {
  return this.setTime(undefined);
};


/**
 * Returns whether this field is set.
 * @return {boolean}
 */
proto.com.book.TimestampType.prototype.hasTime = function() {
  return jspb.Message.getField(this, 1) != null;
};

I didn't find the toDate fromDate you mentioned.

@jonahsmith
Copy link
Author

@agreatfool I just added an example of how this gets triggered for illustration. I would certainly appreciate any input you have on how you'd prefer this functionality to be architected! In particular, it sounds like you'd prefer these to exist as partials, so I suggested an example directory structure for this that could use your input.

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

Successfully merging this pull request may close these issues.

2 participants