How would I write “either” and “withDefault” decoders?

[zoul]

Hi! Thank you for this perfect library, I’m thrilled to have something better than just JSON.parse and praying. I’ve got two use cases I don’t know how to go about, though.

“Either” field decoder – I am decoding a record and want to decode a slug field that contains a string. That’s easy, but if the field’s missing, I want to use the id field instead:

decodeFoo({id: "123", slug: "foo"}) // {id: "123", slug: "foo"}
decodeFoo({id: "123"}) // {id: "123", slug: "123"}

How can I do that without some kind of postprocessing? I’ve tried a few things to make something like either(field("slug", string), field("id", string)) work, but since the field decoder is treated as special in the record decoder, I didn’t get anything working.

And the second case is something like withDefault(boolean, false) decoder. If the either decoder works, would this simply be either(boolean, just(false)) with a helper just decoder that just returns a value?

Thank you!

[tskj]

Hi! Thanks for the kind words and for using the library! I'll do my best to answer your questions, feel free to correct any misunderstands I have of your problem.

If I understand your first case correctly, I think it is as simple as:

const decodeFoo = record({
  id: string,
  slug: fields({ id: string, slug: optional(string) }, ({ id, slug }) => slug ?? id),
});

Kind of annoyingly, you have to specify id twice, but I don't immediately see a good solution to this minor issue.

You were on track with the field or fields decoder! But as you mentioned, those are specially handled by the record decoder, and only really make sense in a record decoder. I think fields specifically is correct for combining two or more fields, which seems to be your requirement here. However, there might be an argument to be made that the code you suggested should have worked, I will think some more about this.

As far as your hypothetical either decoder goes, I think that is more or less exactly what union does (?), except of course it doesn't understand field.

So yes I belive you are spot on that withDefault makes a lot of sense being implemented as simply union(boolean, just(false)). I've considered for some time adding a constant decoder to the library, but haven't - in fear of introducing features nobody use. So this is a good data point that that would be useful!

For reference, this would be an example implementation:

const just = <T>(x: T): Decoder<T> => (_: Pojo) => x

Not quite sure how thîs would best interact with missing fields in a record, though. Look forward to hearing your feedback!

[zoul]

Perfect, much appreciated! The fields decoder is the right solution, thank you.

And d-oh, you’re right about either being union, this late Friday evening hacking 🤦‍♂️ I think the union(boolean, just(false)) decoder works fine by itself, but the record decoder is hard-wired to only one scenario for missing keys, isn’t it?

if (!value.hasOwnProperty(key)) {
  if ((decoder as any)[optionalDecoder]) {
    return [key, undefined];
  }
  throw `Cannot find key \`${key}\` in \`${JSON.stringify(value)}\``;
}

So if the key is missing, the “default value decoder” doesn’t even get a chance to run. Even if I cheated a bit and tagged the default value decoder as optionalDecoder, the returned value is hard-coded as undefined above. Would it make sense to change the record decoder to give the “field decoder” chance to run before trying the optional decoder? (Sorry, have to run now, will flesh out the idea more later.)

[zoul]

So how about something like this in the implementation of record?

if (!value.hasOwnProperty(key)) {
  // If this is an optional decoder, it’s fine to return undefined
  if ((decoder as any)[optionalDecoder]) {
    return [key, undefined];
  } else {
    try {
      // Not an optional decoder, but maybe the decoder can handle undefined?
      // This is what should make things like `union(boolean, just(false))` work.
      return [key, decode(decoder)(undefined)];
    } catch (_) {
      // No, it can’t, so there’s nothing left but to throw
      throw `Cannot find key \`${key}\` in \`${JSON.stringify(
        value,
      )}\``;
    }
  }
}

I would try to implement this and send a PR, but I’m not sure how the test suite works. I think the package makes a lot of sense and it would be nice to have a standard Jest test suite – to make it easier to contribute and maintain. It’s an extra dependency, but only at develop time. Would you consider changing the tests to a Jest test suite? (It’s perfectly fine if not, for whatever reasons.)

[tskj]

Very interesting suggestions!

I have another TypeScript library where the test suite, which can be found here: https://github.com/tskj/typescript-calendar-date/blob/master/specs/index.tests.ts, uses a combination of Jest and fast-check, which I have been very happy with. However, I haven't been able to figure out how to set it up for this library, since what I would like to test is if the TypeScript compiler infers the expected types (while Jest is a runtime JS thing). However I will take another look at this, as good tests are obviously very important, to have faith in the library.

As far as your concrete suggestion goes, I think maybe the cleaner solution then is to drop the entire optionalDecoder tag and just let any decoder try to decode it, where missing keys then look like undefined (optional will still just work under this regime). I'm not sure if I considered this when first making it or not, but this certainly seems like a simpler solution than the current situation.

However, this now doesn't allow any mechanism I can see for people to fail their decoders if the key is missing, and that is what they want. So I am on the fence about making this change, but I might be missing something here. On the other hand, maybe making this distinction at all between missing keys and explicit undefined keys is a mistake, not sure if that actually is a valid use case when I think more about it.

In either case, another way to do what you want might be something like this:

record({
  foo: optional(boolean, x => x ?? false),
})

I have been considering whether more of the combinators (than just field and fields) should support this kind of "continuation passing style" for allowing further transformations. I'm guessing a lot of people not too familiar with parser combinators or similar might find this more intuitive to understand than the union(decoder, defaultConstantDecoder) pattern. Although that would still be available of course, in other contexts than missing keys.

Look forward to hearing your opinions on both topics.

[zoul]

I’ve split the test suite discussion into another issue to keep things tidy.

As for the second point, I did not realize that ({foo:undefined}).hasOwnProperty("foo") returns true, ie. that JavaScript treats completely missing keys differently from keys that are present, but undefined. I can’t think of a use case where this would make sense – especially since we already have null to signal an explicit “empty” value. So I would like to try what you suggest – completely dropping the explicit optional decoder branch (here) and just give the decoder a chance to run regardless of its type. Does that make sense?

And as for the third point (transforming the decoded values), I have to say that’s something that I was looking for when starting to use the library. It feels quite natural and feels like a good middle ground when you want to transform the value a bit, but don’t want to introduce a new custom decoder. (Let’s maybe extract this branch of the discussion into a separate issue, too?)

[tskj]

Good idea splitting the issues up, I have created another issue here.

It seems to be as simple as deleting that if check, and I'm all for it! It almost doesn't seem like a breaking change, which is very good. The only reason I'm slightly hesitant is that we would be removing any possibility for the user to detect missing keys (or more precisely, distinguishing missing keys from undefined keys). While I agree that this is a pathological usecase, there might (or maybe not?) be APIs out there which encode different meanings in missing keys vs undefined keys. I just think we should provide some escape hatch in this pathological case juust in case, but I might be overthinking this.

[zoul]

It’s a quite complex topic, isn’t it? To recap:

There are three possible ways to express some notion of a “missing value” – nulls, undefined and missing keys. Javascript (unfortunately) really does make a difference between all these. And we already have ways to express them here – the nil decoder, the undef decoder and the optional decoder in combination with record.

Our major use case is JSON, where there’s no explicit undefined, so we only have nulls and missing keys. And there’s a legitimate reason to mix those. For example if I want to send a PATCH request to change an object, there‘s a difference between setting a key to null (“I want to clear this field”) and leaving a key out (“I don’t want to touch this field”). Let’s take an example:

interface Contact {
  email?: string
  phone?: string
}

If I want to clear the email field, but keep phone unchanged, there’s only one reasonable way to say that in JSON:

{"email": null}

In JavaScript, that could be either {email: null, phone: undefined} or {email: null}. As far as I understand, the library currently only produces the first version with explicit undefined props (see #3). The request definition would then look like this:

const decodeUpdateRequest = record({
  email: optional(union(string, nil)),
  phone: optional(union(string, nil)),
});

// As inferred by decodeType<typeof decodeUpdateRequest>
type UpdateRequest = {
    email: string | null | undefined; // string: set new value, null: clear value, undefined: no change
    phone: string | null | undefined;
}

Does that make sense?

Now, I’m still looking for a legitimate use case where the object being decoded makes a difference between missing and undefined keys. It can’t be JSON, but since JavaScript makes the difference, it makes sense to have the escape hatch, maybe for decoding values from other sources?

Would it make sense the repurpose the current tagging mechanism used for optional decoder? Say we introduce a missing decoder that’s tagged and special-cased in record and only succeeds when the key is missing? We wouldn’t have to do it now, it should be possible to introduce it later if someone really needs it. And then it would get precedence over the ordinary optional decoder. So it could be a non-breaking change.

[tskj]

Thanks for clearing this up for me, I actually didn't know undefined wasn't a legal JSON value! I had to look that up.

missing seems like a reasonable compromise as an escape hatch as well, good idea! But yeah we can push on that, no reason to that immediately.

Do you want to submit a PR for this? If not I will give it a go shortly.

[JakobBruenker]

So yes I belive you are spot on that withDefault makes a lot of sense being implemented as simply union(boolean, just(false))

Note that (I think) this is actually different from optional(boolean, x => x ?? false) – the former will match anything the server might return, whereas the latter will throw an error if the server returns e.g. a string.

I'm currently using union(decoder, value => undef(value) ?? defaultValue, but #6 does look a bit nicer.

[zoul]

Ah, yes, there are two possible default value semantics: 1) use default when the main decoder doesn’t succeed, or 2) use default when the value is missing.

[tskj]

I think I will get on #6 shortly, then! Seems like people think it's a good idea and would be valuable.

As for withDefault, yes I think (1) use default value when main decoder fails, is the most desirable behaviour.

[JakobBruenker]

use default value when main decoder fails, is the most desirable behaviour.

It might be, generally speaking, though personally, I prefer (2) for my use cases. If the server unexpectedly changed the API I'd rather get an error than blindly using the default value.

[tskj]

Ah, gotcha. I think we are in agreement! Let me nuance my position a bit:

I think it makes the most sense for withDefault in particular to be a way for people to "catch" failling decoders, and in that way create a decoder that can never fail. However, as you say, that is not what you typically want when you use optional on a record field, you actually do want it to fail sometimes. So it's a different usecase, and a more common one at that, which is why I haven't implemented withDefault.

Another argument for only catching exceptions (failling decoders) and not undefined or null values, is that I would rather not have the library mandate a sentinel "fail" value on the user's behalf. You would then have to answer the question, should it be undefined, null, or borth, or empty string and the number 0, not to mention false? So for your usecase I believe the explicit optional(decoder, x => x ?? defaultValue) would be the best solution.

By the way props on your current solution, that's pretty ingenious! Had to take a few moments to reason through how it works to convince myself it actually behaved as expected.

[JakobBruenker]

Heh, thanks :) It does sound like we agree, yeah