castle icon indicating copy to clipboard operation
castle copied to clipboard

avro-ts: Improvements on default values

Open pylebecq opened this issue 4 years ago • 9 comments

Hello 👋

I noticed something that I believe can be improved with default values. When using this Avro schema:

{
  "type": "record",
  "name": "CreateUser",
  "namespace": "my.namespace.messages",
  "fields": [
    {
      "name": "userId",
      "type": "string",
      "logicalType": "uuid"
    },
    {
      "name": "firstname",
      "type": ["string", "null"],
      "default": "John"
    },
    {
      "name": "lastname",
      "type": ["null", "string"],
      "default": null
    },
    {
      "name": "email",
      "type": "string",
      "default": "[email protected]"
    }
  ]
}

If I try to use it with avsc, I can supply only a userId for this object and this is valid because all other fields have default values:

import avsc from "avsc";
import path from "path";
import { readFileSync } from "fs";

const type = avsc.Type.forSchema(
  JSON.parse(
    readFileSync(path.resolve(__dirname, "../avro/CreateUser.avsc")).toString()
  )
);

// This is valid using avsc because all other fields have default values
const buffer = type.toBuffer({ userId: "123456789" });
console.log(buffer);

However, generating typescript definitions from this schema result of the following:

/* eslint-disable @typescript-eslint/no-namespace */

export type CreateUser = MyNamespaceMessages.CreateUser;

export namespace MyNamespaceMessages {
  export const CreateUserName = "my.namespace.messages.CreateUser";
  export interface CreateUser {
    userId: string;
    name: string;
    email: null | string;
  }
}

This means I must provide all keys when trying to create such an object. Otherwise, if I supply only the userId as I did with avsc:

import { CreateUser } from "./__generated__/CreateUser.avsc";

const createUser: CreateUser = { userId: "123456789" };

I get the following error:

src/index.ts(17,7): error TS2739: Type '{ userId: string; }' is missing the following properties from type 'CreateUser': name, email

Is there any reason why the fields with default values are not generated as optional using ?.

pylebecq avatar Jul 30 '20 14:07 pylebecq

@pylebecq can you check to see if it fixed this particular issue?

ivank avatar Aug 04 '20 09:08 ivank

My use case desires the opposite behavior for default values. In my use case, i'm consuming CreateUser documents from my datastore and know that because of the default values in the schema, the returned value must provide values for userId, name, and email. i.e. the object that I get back is of type { user: string; name: string; email: string } and not of type { user?: string; name?: string; email?: string }

awinograd avatar Aug 05 '20 22:08 awinograd

Hmm ... ok that's weird, because if you have a default it is optional ... apart from providing 2 distinct type for input / output, not sure how it could be resolved.

ivank avatar Aug 06 '20 07:08 ivank

For now i've downgraded to 4.x. I think a couple paths forward are:

  1. Keep 5.x behavior and tell people to use NonNullable<GeneratedType> to mimic 4.x behavior in their code
  2. Add a flag to choose how to handle defaults

awinograd avatar Aug 06 '20 16:08 awinograd

Yeah I think I'll go with the flag. That way if you want to use both, you can generate 2 sets of types if you need to.

ivank avatar Aug 07 '20 07:08 ivank

Hopefully this would address everyone's use cases https://github.com/ovotech/castle/pull/47

ivank avatar Aug 07 '20 07:08 ivank

@ivank #47 would satsify my use case, thanks!

I dont need to generate 2 sets of types, but I'm wondering how you recommend going about that for users who do. Wouldn't the namespaces/interfaces clash in terms of naming?

awinograd avatar Aug 07 '20 17:08 awinograd

Wouldn't the namespaces/interfaces clash in terms of naming?

Possibly. This was a quick and dirty solution to make it solvable, but it’s far from the “pit of success” that would be needed.

Since ts offers rich ways to rename imports I doubt it would be a problem, just ... inconvenient.

ivank avatar Aug 08 '20 13:08 ivank

@ivank I believe you are right. I thought namespaces would globally merge across files but that apparently doesn't happen by default! https://www.typescriptlang.org/docs/handbook/namespaces.html#multi-file-namespaces

This is valid

// a.ts
export namespace AvroDemo {
  export interface Foo {
    name: string;
  }
}
// b.ts
export namespace AvroDemo {
  export interface Foo {
    name?: string;
  }
}

awinograd avatar Aug 08 '20 14:08 awinograd