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

Flat oneofs with type union #1145

Open
deser opened this issue Dec 4, 2024 · 8 comments
Open

Flat oneofs with type union #1145

deser opened this issue Dec 4, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@deser
Copy link

deser commented Dec 4, 2024

Hey currently oneof have 3 variants and none of them produce correct type for me. I would need the true union to be generated. See the example below:
Having

message Some {
  oneof doesntmatter {
    SomeStructureA fielda = 1;
    SomeStructureB fieldb = 2;
  }
}

Generated TS will be:

type Some = {fielda: SomeStructureA} | {fieldb: SomeStructureB}
@stephenh
Copy link
Owner

stephenh commented Dec 4, 2024

Hi @deser ; at first I thought that was our default oneof output, in that the fields stay top-level, but, right, we don't technically do the |...

That might be easy-ish to add, since it is similar to the default/flat fields output, if you'd like to attempt a PR. Thanks!

@stephenh stephenh added the enhancement New feature or request label Dec 4, 2024
@stephenh stephenh changed the title [Feature request] Add new oneof option Flat oneofs with type union Dec 4, 2024
@deser
Copy link
Author

deser commented Dec 5, 2024

I'm afraid it's not that simple, consider nested oneof...

syntax = "proto3";

message OuterMessage {
  oneof outer_field {
    InnerMessage inner_message = 1;
  }
}

message InnerMessage {
  oneof inner_field {
    string string_value = 1;
    int32 int_value = 2;
  }
}

I peeked at the code, seems a new chunk of code will need to be written there (I wouldn't be able to re-use existing logic handling oneof as it just generates property in specific way, when with new union type it is required to generate new type for every oneof value), without knowing the code base, it will take considerable amount of time from myself :)

@stephenh
Copy link
Owner

stephenh commented Dec 5, 2024

What do you expect the type for your OuterMessage to look like?

it will take considerable amount of time from myself

Yep, that's how open source goes. :-)

@deser
Copy link
Author

deser commented Dec 5, 2024

I think somehow like below:

type OuterMessage = {} | {inner_message : {string_value: string}}  | {inner_message : {int_value : number}}

@stephenh
Copy link
Owner

stephenh commented Dec 5, 2024

I was thinking it'd be something like:

type OuterMessage = {} | {inner_message : InnerMessage }

type InnerMessage = { string_value: string } | { int_value: number }

Which I believe is "the same type" but would be a lot easier to output, given ts-proto's current "message oriented" output.

@deser
Copy link
Author

deser commented Dec 5, 2024

Yes, there's also possibility to put shared props into its own common interface, but that might affect performance, so duplicating is probably ok

@benlesh
Copy link

benlesh commented Dec 11, 2024

FYI: For anyone landing here looking for a workaround, you can use this type to break N optional properties on an interface into a union of required (mutually exclusive) properties.

type ExclusiveUnion<T> = NonNullable<{
  [K in keyof T]: {
    [P in K]-?: T[P];
  }
}[keyof T]>


type Test = ExclusiveUnion<{ foo?: number, bar?: string }>; // { foo: number } | { bar: string }

Here's a TS Playground: https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgApQPZgwWQgZ3zgHMUBvAKGWuRgwwH4AuZAMXoG4qaAjOKZsgBC-LjWR8AXoJGSuAXwoVQkWIhTsMySuLoYAInDBwWIAK4BbHtADaAXQVKV0eEmH9t3anygA5OBYQLPhgUKDEYrwQ0OjASKaW1lCOyuAu6u6SnuJSkgCeACoAFuH4waHh9hSKFGB5AA4oAKIAHggANmb4wABuEACqIMAYIAA8BQB8yAC8yL4jvmbt7XA87RCjOsg2ANLIoMgA1hB5GDDIBXYsW9Q2qPsgyDt2ALSCBXcO3IryNsen50uEyUtQaKAAgggwGY4O10FhcAQiKQZshWh0ur0BkMRqN4dg8IQSBAJkA

@stephenh
Copy link
Owner

Yes, there's also possibility to put shared props into its own common interface,
but that might affect performance, so duplicating is probably ok

I'm not sure why you wouldn't put the InnerMessage fields in their own type; it's very idiomatic for each message Whatever in a *.proto file to have a type Whatever in the TS code, and InnerMessage being used within a OuterMessage oneof imo shouldn't change that.

Fwiw I would probably try to get the types to be:

type OuterMessage = OuterMessageRegularFields & OuterMessage_OuterFieldA & OuterMessage_OuterFieldB;

type OuterMessageFieldA = { fielda: ... } | { fieldb: ... }

type OtherMessageFieldB = { fieldc: ... | | { fieldd: InnerMessage }

Just to break things down a little bit, for when OuterMessage invariably has multiple oneofs. 🤷

Fwiw @benlesh I don't totally follow your example, as I believe it assumes that OuterMessage only has a single oneof; as soon as there are multiple oneofs, that approach will break, right?

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

No branches or pull requests

3 participants