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

taggedUnion tag / discriminator isn't typed #1153

Open
dearlordylord opened this issue Jul 28, 2024 · 2 comments
Open

taggedUnion tag / discriminator isn't typed #1153

dearlordylord opened this issue Jul 28, 2024 · 2 comments
Labels

Comments

@dearlordylord
Copy link

dearlordylord commented Jul 28, 2024

Description of the bug

taggedUnion tag / discriminator is typed as a "string" not checking against field existence / field (constant) content

both parses would fail

Expected behavior

this shouldn't compile

To Reproduce


// note there's a typo in "type" (copy-paste it or write gibberish yourself to reproduce)
const profileDecoder1 = taggedUnion('tуpe', {
  listener: object({
    type: constant('listener'),
    boughtTracks: number,
  }),
  artist: object({
    type: constant('artist'),
    publishedTracks: number,
  }),
});

const profile = profileDecoder1.decode({ type: 'listener', boughtTracks: 10 });

console.log(profile.ok === false);

// no typo in "type" but artist/listener are switched
const profileDecoder2 = taggedUnion('type', {
  artist: object({
    type: constant('listener'),
    boughtTracks: number,
  }),
  listener: object({
    type: constant('artist'),
    publishedTracks: number,
  }),
});

const profile2 = profileDecoder2.decode({ type: 'listener', boughtTracks: 10 });

console.log(profile2.ok === false);

Additional context
Decoders version: 2.4.2
TypeScript version: 5.4.5
Flow version: ?
Node version: browser

@nvie
Copy link
Owner

nvie commented Jul 30, 2024

Hi @dearlordylord – I just took a look at your snippet, but both seem to work as expected. Maybe I'm missing your point? The first snippet declares you have an object literal that is expected to have a field "tуpe" (with typo as y char is non-ascii), but you're giving it a field named "type" (no typo). Hence the decoder fails correctly:

Decoding error:
{
  "type": "listener",
  "boughtTracks": 10,
}
^ Missing key: 'tуpe' 👈 Note the "y" here is non-ascii

In your second snippet, you flip the order of "type" field literals. Unfortunately, this is never going to work, as the first layer of checks will decide which decoder to use (in your case, it will look up which decoder to use for the "listener" field), and use that decoder on the input again. But since that decoder has defined type: constant("artist") instead, this will always fail.

This may seem a bit un-DRY at first, but it's a necessary duplication because of TypeScript and runtime limits. If you care about the DRY-ness, you could use either() instead. However, I'd still recommend taggedUnion (including these downsides!) in order to have much better error messaging and runtime performance.

Let me know if this answers your question.

@dearlordylord
Copy link
Author

Thank you for taking time to explain this. I want to add that type-level DRY doesn't concern me as much as runtime-level DRY; if I have to repeat myself, but I'm also checked by the compiler, it's completely fine.

Here I'm not checked by the compiler though as I can write whatever into the discriminator field (I'll call it discriminator because it's more common name, e.g. https://www.typescriptlang.org/docs/handbook/2/narrowing.html#discriminated-unions references it )

I'm also coming from

https://github.com/effect-ts/effect/tree/main/packages/schema#discriminated-unions

https://github.com/colinhacks/zod?tab=readme-ov-file#discriminated-unions

https://gist.github.com/ssalbdivad/d60d876ab6486adc97e38e3f6916e93f

who seem to overcome typescript limitations and make it "not compile" on wrong inputs provided

So, to reiterate, I'm fine with

taggedUnion('type', {
  listener: object({
    type: constant('listener'),
  }),
  artist: object({
    type: constant('artist'),
  }),
});

but I'm not fine with

taggedUnion('type', {
  listener: object({
    type: constant('listener'),
  }),
  artist: object({
    type: constant('artist'),
  }),
});

or with

taggedUnion('uWu', {
  listener: object({
    type: constant('listener'),
  }),
  artist: object({
    type: constant('artist'),
  }),
});

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

No branches or pull requests

2 participants