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

Finish oneof support #48

Closed
melonmouse opened this issue May 20, 2021 · 4 comments · Fixed by #53
Closed

Finish oneof support #48

melonmouse opened this issue May 20, 2021 · 4 comments · Fixed by #53
Labels
help wanted Extra attention is needed

Comments

@melonmouse
Copy link

Thanks so much for working on this project! Hope this bug will help :). I looked at your code for a bit to see if I could write you a pull request, but wasn't sure where to start.

To complete oneof support, there should be helper functions to determine what case is set.
https://developers.google.com/protocol-buffers/docs/reference/javascript-generated#oneof

So taking the example from above:

package account;
message Profile {
  oneof avatar {
    string image_url = 1;
    bytes image_data = 2;
  }
}

There should be a generated enum representing the cases, so for the above example:

proto.account.Profile.AvatarCase = {
  AVATAR_NOT_SET: 0,
  IMAGE_URL: 1,
  IMAGE_DATA: 2
};

And a getter that returns the enum value that is currently set, getAvatarCase().

@melonmouse
Copy link
Author

P.S. if it is any help, if you'd write a "pseudo code" commit adding this functionality, I could complete that. Not sure if it would save you time, but wanted to offer :)

@thesayyn
Copy link
Owner

Hey, thank you for opening this. We had a brief conversation on another issue.

It makes more sense to continue the conversation here.

P.S. if it is any help, if you'd write a "pseudo code" commit adding this functionality, I could complete that. Not sure if it would save you time, but wanted to offer :)

I can assist you on the way if you could open up a draft pull request.

@thesayyn thesayyn added help wanted Extra attention is needed priority: 1 labels Jun 1, 2021
@thesayyn
Copy link
Owner

thesayyn commented Jun 2, 2021

Hey @melonmouse

When you have some time, could you join the discussion at #39 about one-of-fields?

@melonmouse I have sent PR for this feature on #53

@jackpunt
Copy link

Sahin, sorry to be late getting to this, I've been off for the summer.
It seems that this needs a bit more work; consider:

message TypedMsg {
  oneof value {
    bool boolValue = 3;
    int32 intValue = 4;
    string strValue = 5;
  }
}

I believe it should be the case that:

let msgInt = new TypedMsg({intValue: 23})
let msgStr = new TypedMsg(strValue: "foo"})
assert( msgInt.intValue === 23)      // works
assert( msgStr.strValue === "foo")   // works
assert( msgInt.value === 23)         // fails: msgInt.value === "intValue"
assert( msgStr.value === "foo")      // fails: msgStr.value === "strValue"

The type of TypedValue.value should be: number | string | boolean | void [or | undefined? i'm not an expert on that ]
rather than: "intValue" | "strValue" | "boolValue" | "none"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants