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 support for union types #39

Closed
ecamellini opened this issue Jan 23, 2019 · 7 comments · Fixed by #62
Closed

Add support for union types #39

ecamellini opened this issue Jan 23, 2019 · 7 comments · Fixed by #62

Comments

@ecamellini
Copy link
Contributor

Metarpheus should support union types such as:

sealed abstract trait UnionExample
object UnionExample {
  case class First(arg: String) extends UnionExample
  case class Second(arg: String, anotherArg: String) extends UnionExample
}

Also opened buildo/metarpheus-io-ts#79 to add the generation to metarpheus-io-ts

@gabro gabro transferred this issue from buildo/metarpheus May 28, 2019
@tpetrucciani tpetrucciani self-assigned this May 31, 2019
@gabro gabro added the BE maint label Jun 7, 2019
@prismabot prismabot added the WIP label Jun 7, 2019
@tpetrucciani
Copy link
Contributor

Currently metarpheus recognizes this pattern:

sealed trait T
object T {
  case object A extends T
  case object B extends T
  case object C extends T
}

as describing an enumerated type T with three values A, B, C. (It also supports the @enum annotation from enumero.) Enumerations are described as a CaseEnum in the intermediate representation.

To add support for union types, we can accept the more general case in which inside object T there are both case objects and case classes. In this case, it's not an enumeration but rather a tagged union type.

I'm ignoring type parameters for now, since we probably don't need them in many use cases.

@gabro
Copy link
Member

gabro commented Jun 17, 2019

👍 it's fine to only infer enums when using the enumero syntax. All other cases should be union types.

@giogonzo
Copy link
Member

I'm ignoring type parameters for now, since we probably don't need them in many use cases.

You probably already discussed this, but: if I got this correctly, it means that a type like e.g. Option = None | Some<T> will not be supported? (not in a generic way at least - this specific case is already hardcoded into metarpheus-io-ts)

Did you also discuss specific examples from real projects where the union types feature would have been useful?

@tpetrucciani
Copy link
Contributor

You probably already discussed this, but: if I got this correctly, it means that a type like e.g. Option = None | Some will not be supported? (not in a generic way at least - this specific case is already hardcoded into metarpheus-io-ts)

Yes, we discussed it with Gabro and @calippo, but it's just a first step – it doesn't seem too difficult to add type parameters later.

I suppose types like Option and Either won't use this feature anyway since we're not translating their definition but mapping them to existing fp-ts types. Therefore I suspect (without much data to back it though 😉) that we won't use very many custom union types with type parameters in typical use cases (like errors, below), but it would be interesting to see examples.


As for examples, we've mostly talked about it with Claudio to use in tapiro to represent errors. But I think I had also run into a real use case in code where we had something like

case class C(
  ...,
  kind: AnEnumeratedType,
  extraInfoForKind1: Option[ExtraInfoForKind1],
  extraInfoForKind2: Option[ExtraInfoForKind2],
  extraInfoForKind3: Option[ExtraInfoForKind3],
  ...
)

where the value of kind determined which of extraInfoN was Some and which were None, so this could have been replaced with a union type. I'll try to find it again.

@tpetrucciani
Copy link
Contributor

A doubt: when we use in our model a tagged union

sealed trait UnionExample
object UnionExample {
  case class First(arg: String) extends UnionExample
  case class Second(arg: String, anotherArg: String) extends UnionExample
}

are we interested in using First and Second as stand-alone types sometimes (e.g. in route or case class parameters) or do we only want to use UnionExample?

This could influence whether to put First and Second in the model as separate case classes or only inside UnionExample and also what to do on the TypeScript side.

Currently in my PR:

  • First and Second are generated in the model as separate case classes (and as parameters of UnionExample)
  • the case classes are then removed when we strip unused models as long as they are never referenced except through UnionExample
  • if they are not removed, then they'll appear twice in the model, once as members of UnionExample and once as stand-alone case classes

Some possibilities:

  1. do not have them as stand-alone case classes (which is already what happens if we don't refer to them, thanks to stripUnusedModels)
  2. have them only as separate case classes and refer to them in the tagged union model rather than duplicating the definition

If we care about them as separate types we might want 2, or just keep the current state. But it's probably important that we have just one type definition in TypeScript.

What do you think about that?

I'd lean more towards keeping it as it is currently and by convention never using First and Second separately (and adding a layer of boxing if we need to). But I don't know for sure if that matches current conventions or possible use cases.

@calippo
Copy link
Member

calippo commented Jun 21, 2019

I suppose we will mainly access First and Second when iterating on UnionExample representations. For the error use case, not having them as stand-alone case classes is fine.

Are we losing any information not having them as stand-alone case classes?

@tpetrucciani
Copy link
Contributor

I don't think we lose any information in doing so, we can just record it all in the members of the tagged union.

I think this can be a first step, we can evaluate later whether we're interested in using the subtypes like First and Second on their own sometimes in the models. (Unless we already know we're interested in using them.)

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

Successfully merging a pull request may close this issue.

7 participants