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

Allow sealed classes to reference external enums #68

Merged
merged 7 commits into from
Jan 9, 2023

Conversation

sam-bunger
Copy link
Collaborator

@sam-bunger sam-bunger commented Jan 5, 2023

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
Copy link

github-actions bot commented Jan 5, 2023

Coverage report

St.
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.
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 e95199e

@asarazan
Copy link
Owner

asarazan commented Jan 6, 2023

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

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
Copy link
Owner

asarazan commented Jan 6, 2023

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
Copy link

Codecov Report

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

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
@@            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 Coverage Δ
src/typescript/MemberHelpers.ts 98.05% <ø> (ø)
src/martok/declarations/TaggedUnionGenerator.ts 99.02% <93.75%> (-0.98%) ⬇️
src/martok/declarations/KlassGenerator.ts 96.45% <100.00%> (ø)
src/typescript/EnumHelpers.ts 94.87% <100.00%> (+0.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@sam-bunger
Copy link
Collaborator Author

@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.

@sam-bunger sam-bunger merged commit 5283ac6 into main Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicated enum type outside union/sealed class
3 participants