sam-bunger commented 1 year ago

Not sure if a PR is a place to request this... BUT there is a feature that would really help Faceoff resolve some duplicate type issues.

Fixes #67 Resolves FACE-1206

The Problem

When Martok transpiles tagged union types, a new enum always gets created inside the generated sealed class, even if we want the tagged union type to reference an enum outside of the sealed class.

For example, our GameSession type is tagged using this GameType enum:

export enum GameType {
  SongQuiz = "songQuiz",
  SketchyAf = "sketchyAf",
  WordRacer = "wordRacer",
}

export type GameSession = {
  id: string;
  callSessionId: string;
  principalUserId: string; /// User who starts the game
  gameState: "started" | "ended";
  startDateTime: string;
  endDateTime?: string;
} & (
    | {
      gameType: GameType.SongQuiz
      specificGameSessionState?: SongQuizSessionState;
    }
    | {
      gameType: GameType.SketchyAf
      specificGameSessionState?: SketchySessionState;
    }
    | {
      gameType: GameType.WordRacer
      specificGameSessionState?: WordRacerSessionState;
    })

When this transpiles we get two enum types, one outside the sealed class, and one inside the sealed class:

@Serializable
enum class GameType(val serialName: String) {
  @SerialName("songQuiz") SONG_QUIZ("songQuiz"),
  @SerialName("sketchyAf") SKETCHY_AF("sketchyAf"),
  @SerialName("wordRacer") WORD_RACER("wordRacer")
}

@Serializable(with = GameSession.UnionSerializer::class)
sealed class GameSession {

  abstract val id: String

  abstract val callSessionId: String

  abstract val principalUserId: String

  abstract val gameState: GameState

  abstract val startDateTime: String

  abstract val endDateTime: String?

  abstract val gameType: GameType

  @Serializable
  enum class GameType(val serialName: String) {
    @SerialName("songQuiz") SONG_QUIZ("songQuiz"),
    @SerialName("sketchyAf") SKETCHY_AF("sketchyAf"),
    @SerialName("wordRacer") WORD_RACER("wordRacer")
  }
...

This makes it difficult because if we want multiple classes to reference the same enum then we'll need to manually delete the enum inside each sealed class.

The Solution

I really don't know! BUT I added in some tests in this PR that test against this use case

github-actions[bot] commented 1 year ago

Coverage report

St.^{:grey_question:}	Category	Percentage	Covered / Total
🟢	Statements	94.22% (-0.1% 🔻)	945/1003
🟢	Branches	88.95% (-0.25% 🔻)	459/516
🟢	Functions	94.36% (+0.06% 🔼)	184/195
🟢	Lines	95.41% (+0.06% 🔼)	893/936

Show files with reduced coverage 🔻

|

St.^{:grey_question:}

| File | Statements | Branches | Functions | Lines | | :----------------------------------------------------------------------------------------------------------------------------------------------------: | :--------------------------------------------------------------------------------------------- | :------------------------------------------------------------- | :------------------------------------------------------------- | :-------- | :---- | | 🟢 |

`...` / TaggedUnionGenerator.ts

|

93.16% (-0.96% 🔻)

|

72.55% (+0.75% 🔼)

| 100% | 100% |

Test suite run success

32 tests passing in 4 suites.

Report generated by 🧪jest coverage report action from e95199ed212926e30da320f0bbea8287c7fba106

asarazan commented 1 year ago

Ok @sam-bunger if you look in TaggedUnionGenerator, you'll see a few interesting lines:

https://github.com/asarazan/martok/blob/main/src/martok/declarations/TaggedUnionGenerator.ts#L92 where we generate the enum that you'd like to skip
https://github.com/asarazan/martok/blob/main/src/martok/declarations/TaggedUnionGenerator.ts#L86 where we create the members that are of that enum type.

Now, the compiler assumes that you're building your tagged unions off string unions, and hadn't considered that you would already have an enum ready to use instead.

What you'll want to do is add some conditional logic around those lines (or inside https://github.com/asarazan/martok/blob/main/src/martok/declarations/TaggedUnionGenerator.ts#L108) which detects when the tag candidate is of enum type, and uses that instead. Unfortunately you'll also then need to override the naming scheme that the compiler uses (title(tag.name) or something like that).

I've got a decent amount of free time tomorrow so hmu and I can walk you through it.

asarazan commented 1 year ago

Also if you debug npm run special and set a breakpoint at line 74, you should be able to look around the execution state and get a good feel for what things needs to be changed.

codecov-commenter commented 1 year ago

Codecov Report

Base: 95.06% // Head: 95.03% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (855758e) compared to base (71f5ec9). Patch coverage: 95.00% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #68 +/- ## ========================================== - Coverage 95.06% 95.03% -0.03% ========================================== Files 28 28 Lines 1073 1087 +14 Branches 250 259 +9 ========================================== + Hits 1020 1033 +13 - Misses 50 51 +1 Partials 3 3 ``` | [Impacted Files](https://codecov.io/gh/asarazan/martok/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan) | Coverage Δ | | |---|---|---| | [src/typescript/MemberHelpers.ts](https://codecov.io/gh/asarazan/martok/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan#diff-c3JjL3R5cGVzY3JpcHQvTWVtYmVySGVscGVycy50cw==) | `98.05% <ø> (ø)` | | | [src/martok/declarations/TaggedUnionGenerator.ts](https://codecov.io/gh/asarazan/martok/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan#diff-c3JjL21hcnRvay9kZWNsYXJhdGlvbnMvVGFnZ2VkVW5pb25HZW5lcmF0b3IudHM=) | `99.02% <93.75%> (-0.98%)` | :arrow_down: | | [src/martok/declarations/KlassGenerator.ts](https://codecov.io/gh/asarazan/martok/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan#diff-c3JjL21hcnRvay9kZWNsYXJhdGlvbnMvS2xhc3NHZW5lcmF0b3IudHM=) | `96.45% <100.00%> (ø)` | | | [src/typescript/EnumHelpers.ts](https://codecov.io/gh/asarazan/martok/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan#diff-c3JjL3R5cGVzY3JpcHQvRW51bUhlbHBlcnMudHM=) | `94.87% <100.00%> (+0.42%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Aaron+Sarazan)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

sam-bunger commented 1 year ago

@asarazan Just added in an extra test, and added a new typeName property onto the tagged union mapping type. This feels like it's good to go, let me know your thoughts.

asarazan / martok

Allow sealed classes to reference external enums #68

Coverage report

Test suite run success

Codecov Report