ovotech / castle

Framework for building Kafka and avro based apps in typescript
Other
78 stars 19 forks source link

avro-ts: Improvements on default values #42

Closed pylebecq closed 4 months ago

pylebecq commented 4 years ago

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": "john.doe@example.com"
    }
  ]
}

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

ivank commented 4 years ago

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

awinograd commented 4 years ago

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 }

ivank commented 4 years ago

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.

awinograd commented 4 years ago

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
ivank commented 4 years ago

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 commented 4 years ago

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

awinograd commented 4 years ago

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

ivank commented 4 years ago

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.

awinograd commented 4 years ago

@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;
  }
}
pylebecq commented 4 months ago

Better late than never, I'm closing this as indeed it now works way better for my use case. Thank you @ivank .