Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

RFC: Problems which we already solved by taking modern approaches and what should we do next #39

Closed
thesayyn opened this issue Apr 19, 2021 · 4 comments

Comments

@thesayyn
Copy link
Owner

thesayyn commented Apr 19, 2021

This issue is here to help people understand why I have created a new plugin that diversified the protocol buffer typescript community even more.

Q: Why would you create a plugin from scratch when you can fix problems on the upstream compiler?

A: Well it is not that simple because there is a lot of bureaucracy to make any of the design changes that I have done with this plugin. Even if we managed to pass that, there would be breaking changes that will not make design changes like this.



Q: Why should I use this plugin when I can use https://github.com/improbable-eng/ts-protoc-gen?

A: Keep in mind that "ts-protoc-gen" generates only d.ts binding which basically a type declaration file for the javascript output that has been generated by the official protoc compiler. This comes with a trade-off of not being able to customize the d.ts output as much as we like. If we did. it wouldn't match with the javascript file and you’d get a runtime error even though the typescript compiler wouldn’t complain about the javascript file being different than the d.ts file. So ts-protoc-gen has to match the javascript file which limits its ability to compile proto files to modern typescript syntax.



Q: How does protoc-gen-ts solve this problem?

A: By throwing away the javascript compiler of the official protoc compiler. Instead, we generate a typescript file (".ts" not "d.ts") which includes the runtime code that will run on the underlying javascript engine when it is running. Hence we have the ability to generate the proto files the way we want them.

There are a number of things that we do differently than the official protoc compiler that complies to javascript such as "generating files as typescript getter setter" which in turn you get the exact names that you have defined in the proto message. For instance, when you have a field named user_messages with the official compiler you'll get getUserMessages(). however, if you use this plugin, you’ll just access the property as is like user_messages.

Also, your enums will be generated as a typescript enum instead of an interface or object with predefined keys.

@thesayyn thesayyn pinned this issue Apr 19, 2021
@thesayyn thesayyn changed the title RFC: Problems which we already solved by taking modern approaches RFC: Problems which we already solved by taking modern approaches and what should we do next Apr 19, 2021
@jackpunt
Copy link

jackpunt commented Apr 25, 2021

Suggestion for "what to do next":
Take a look at the code generated for 'oneof' messages.
The spec says that setting one value will un-set the other values. (so only one value will ever be serialized)
Also, spec suggests creating a '...Case' enum field, with value indicating which 'oneof' values is currently set.
The Dart language version looks likes a good cultural fit for protoc-gen-ts (the java & js productions are ugly...)

ultimately, for:

message TypedMsg {
 // enum { null=0; bool=1; int=2, str=3 } valueType = 1
  oneof variant {
    bool boolValue = 2;
    int32 intValue = 3;
    string strValue = 4;
  }
} 

It's called TypedMsg, because currently I include a field with an enum to tell me what type was actually sent.
Now that I've read the spec on 'oneof', I see that should be unnecessary.

Should be able to:

let tm = TypedMsg.deserialize(bytes)
let variant: boolean | number | string = tm.variant

and if I actually want to confirm the type that was received, use a generated: enum TypedMsg_variant = {...}

let tm_case: TypedVariant_value = tm.whichVariant // readonly; 
switch (tm_case) {
 case TypedMsg_variant.boolValue { ... }
 case TypedMsg_variant.intValue { ... }
 case TypedMsg_variant.strValue { ... }
 case TypedMsg_variant.notSet { ... }
}

I suspect the main thing is to include an enum-valued hidden field like:

_whichVariant: TypedMsg_variant
get whichVariant():TypeValue_variant { return this._whichVariant; }

and for each field:

set field(value: ${field_type}) { 
   pb_1.TypedMsg.setField(this, $n, value);
   this._whichVariant = TypedMsg_variant.${field_type}
}

And either actually clear other fields OR arrange the field getters test _whichVariant and returns undefined, which suffice to ensure that serialize will only find/output one field.

Maybe the protoc "extension library" has support for "oneof" ??
If you give me some pointers, I may be able to make a PR...

... [edit]
After further review (looking at generated js and java code), it is not standardized;
js maintains separate fields for each variant type, and uses jspb.Message.computeOneofCase and this.oneofgroup[...]
java uses a single field, (like _value: any) and uses _whichType to achieve the doc'd effects
in java they are named:

   private int valueCase_ = 0;
   private java.lang.Object value_;

@thesayyn
Copy link
Owner Author

thesayyn commented Jun 1, 2021

@jackpunt I just time to start working on this one. Creating a new enum for each of these cases and one-of-declarations seemed like overkill to me.

I feel like we need something more practical

My suggestion is to generate two more additional getters for an individual one-of-declaration

given;

message Person {
    oneof name {
       string nick = 1;
       string real = 2;
       string fake = 3;
    }
}

would look like below

class Person {
    get nick(): string;
    set nick(): string;
    get real(): string;
    set real(): string;
    get fake(): string;
    set fake(): string;
    get name(): string | string | string;
    get which_name(): "nick" | "real" | "fake" | "none";
}

Or

class Person {
    set nick(): string;
    set real(): string;
    set fake(): string;
    get name(): string | string | string;
    get which_name(): "nick" | "real" | "fake" | "none";
}

@jackpunt
Copy link

jackpunt commented Jun 2, 2021

Ok, sure.
The main requirement is to have: get name(): union_of_types
Doesn't really matter whether the discriminator returns field_name: string or field_id: number or some enum...
As long as it can be run through a switch/case, it should work

@thesayyn
Copy link
Owner Author

thesayyn commented Jun 3, 2021

@jackpunt checkout #48

Repository owner locked and limited conversation to collaborators Jun 16, 2021
@thesayyn thesayyn unpinned this issue Jun 16, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants