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

Should decoders match their decoded type exactly, or should there be multiple valid decoders for a type? #23

Open
mulias opened this issue Aug 16, 2018 · 11 comments

Comments

@mulias
Copy link
Contributor

mulias commented Aug 16, 2018

Recently I've been exploring ways to require that the optional decoder be used for fields that are optional. From that process I've realized that I need to make a number of subtle design decisions related to how loosely or strictly a decoder needs to match a type. On the one hand, we should leverage typescript to help us write decoders that are as accurate as possible. On the other hand, I don't want the rules/guidelines around writing decoders to get too complicated, and I also don't want to be overbearing and prevent the user from writing the exact decoder they intend to.

With all that in mind, I've got a few examples of situations where I could change the library to more strictly fit decoders to types. Please respond with feedback to the three questions, plus any other concerns or observations you've got.

  1. Here are four decoders for the interface AB. All four decoders are valid and will compile without errors. In an ideal world, which of these decoders would you want to be valid, and which ones should produce an error?
interface AB {
  a: string;
  b?: number;
}

const decoderAB1 = object({
  a: string(),
  b: optional(number())
});

const decoderAB2 = object({
  a: string(),
  b: number()
});

const decoderAB3 = object({
  a: string(),
  b: union(number(), constant(undefined))
});

const decoderAB4 = object({
  a: string()
});
  1. Ditto for CD. All four decoders are valid, but as a library user which ones would you want to be valid?
interface CD {
  c: string;
  d: number | undefined;
}

const decoderCD1 = object({
  c: string(),
  d: optional(number())
});

const decoderCD2 = object({
  c: string(),
  d: constant(undefined)
});

const decoderCD3 = object({
  c: string(),
  d: union(number(), constant(undefined))
});

const decoderCD4 = object({
  c: string()
});
  1. Ditto for E.
interface E {
  e: string | number;
}

const decoderE1 = object({
  e: union(string(), number())
});

const decoderE2 = object({
  e: string()
});
@mulias
Copy link
Contributor Author

mulias commented Aug 16, 2018

To tip my hand, I think it's reasonable to say that decoderAB1, decoderCD3, and decoderE1 should be the only valid decoders. That said, I don't think there's a way to restrict the decoder for E for the general union case.

@mulias mulias changed the title Should decoders match their decoded type exactly, or can there be multiple valid decoders for a type? Should decoders match their decoded type exactly, or should there be multiple valid decoders for a type? Aug 16, 2018
@RocketPuppy
Copy link
Contributor

AB1, AB2, CD2, CD3, E1, E2.

I'm aiming for a decoder that outputs values that are more specific or equal to the types defined. So, AB1 matches the type exactly. AB2 is more specific, in that a required number will always typecheck against an optional number. CD2 is more specific for a similar reason. A member of a branch will always typecheck against a union of itself. CD3 matches exactly. E1 matches exactly. E2 is the same as CD2.

@mulias
Copy link
Contributor Author

mulias commented Aug 16, 2018

@RocketPuppy That might actually be the most balanced solution.

It sounds like you think that optional(...) and union(..., constant(undefined)) should be separate decoders that are used in different situations. My intention was that those two decoders produce different output like this:

decoderAB1.runWithException({a: 'an string'}) // => {a: 'an string'}
decoderAB3.runWithException({a: 'an string'}) // => {a: 'an string', b: undefined}

Strictly speaking both of those are valid AB objects, since b?: number is implicitly b?: number | undefined. I suspect that in most situations the distinction doesn't matter, but in order to exactly match the meaning of b?: in TS we need two different decoders.

@RocketPuppy
Copy link
Contributor

Right, they aren't really distinct in Javascript, but they are distinct in subtle ways within Typescript.

@geraldalewis
Copy link

@RocketPuppy maybe I'm missing your point there, but they're distinct in JS -- e.g., the in operator.

const ab = {a: true, b: undefined};
'b' in ab; // true
'c' in ab; // false

@RocketPuppy
Copy link
Contributor

I didn't know about the in operator. I don't think I've ever used it. That's consistent with Typescript's semantics as well. My point is that you notice things like that in Typescript more often since you have the fast feedback of the type system. In JS I think it's much more common to just treat a missing key and a key with a value of undefined as the same, e.g. conditional checks like if(foo.x) or if(foo['bar']).

@mulias
Copy link
Contributor Author

mulias commented Aug 22, 2018

Object.keys will also return different results

@geraldalewis
Copy link

geraldalewis commented Aug 23, 2018

interface AB {
  a: string;
  b?: number;
}
const decoderAB1 = object({
  a: string(),
  b: optional(number())
});

AB1 is my preferred decoder.

const decoderAB2 = object({
  a: string(),
  b: number()
});

AB2 is wrong and should throw.

const decoderAB3 = object({
  a: string(),
  b: union(number(), constant(undefined))
});

... This one is tricky -- AB3 is debatable. in checks would fail here if we cared about checking for presence of a prop. I think you should allow it.

const decoderAB4 = object({
  a: string()
});

I think, to keep consistent with TypeScript, AB4 should also be allowed.

type A = { a: boolean; };
const ab = { a: true, b: false };
const fa = (a: A) => a.a;
const t = fa(ab); // true

@geraldalewis
Copy link

interface CD {
  c: string;
  d: number | undefined;
}
const decoderCD1 = object({
  c: string(),
  d: optional(number())
});

I think CD1 is okay. I don't think it should throw; it's technically incorrect though (and will break in and Object.keys.

const decoderCD2 = object({
  c: string(),
  d: constant(undefined)
});

CD2 should throw

const decoderCD3 = object({
  c: string(),
  d: union(number(), constant(undefined))
});

CD3 is my preferred decoder.

const decoderCD4 = object({
  c: string()
});

CD4 is okay by me (for the same reason AB4 is okay by me).

@geraldalewis
Copy link

interface E {
  e: string | number;
}
const decoderE1 = object({
  e: union(string(), number())
});

E1 is my preferred decoder.

const decoderE2 = object({
  e: string()
});

E2 should error.

@geraldalewis
Copy link

Sorry for the spam comments, esp. since I'm re-evaluating (I hadn't updated my mental model and was interpreting the interfaces as e.g. the shape of data returned from an api).

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

No branches or pull requests

3 participants