avsc icon indicating copy to clipboard operation
avsc copied to clipboard

Using seprately declared enum in union in record.

Open svirpridon opened this issue 1 year ago • 1 comments

Hi,

I have a use case where I have an enum I want to declare outside the record. It works fine if I'm not using a union:

const enum = avro.Type.forSchema({
  name: "enum",
  type: "enum",
  symbols: ["foo", "bar", "baz"],
});

const record = avro.Type.forSchema({
  name: "record",
  type: "record",
  fields: [
    { name: "enum", type: enum },
  ],
});

But when I go to make it nullable with:

const record = avro.Type.forSchema({
  name: "record",
  type: "record",
  fields: [
    { name: "enum", type: ["null", enum], default: null },
  ],
});

I get a type error pointing to the enum in ["null", enum]:

error TS2322: Type 'Type' is not assignable to type 'DefinedType'.
  Type 'Type' is not assignable to type 'FixedType & LogicalTypeExtension'.
    Type 'Type' is missing the following properties from type 'FixedType': type, size

If I inline the union it works fine... but I don't want to do that. It'd work for one of my unions, but another one is a union of a couple thousand entries, and I need it in multiple places so that'd get unwieldy fast.

I also tried using the name of the enum as a named type in the record, but that doesn't seem to work.

Thanks for the useful library!

svirpridon avatar May 27 '24 02:05 svirpridon

Hi @svirpridon. This is an issue with the typings, the underlying library supports this. Until this is fixed, it is safe to use an any cast here. Alternatively, you can use the name approach you mention by using the registry option:

const enumType = avro.Type.forSchema({
  name: "Enum",
  type: "enum",
  symbols: ["foo", "bar", "baz"],
});

const recordType = avro.Type.forSchema({
  name: "Record",
  type: "record",
  fields: [
    { name: "enum", type: ["null", "Enum"], default: null },
  ],
}, {registry: {Enum: enumType}});

mtth avatar Jun 21 '24 09:06 mtth

@mtth can't we just add | Type to DefinedType for this?

joscha avatar Sep 19 '24 10:09 joscha

@mtth can't we just add | Type to DefinedType for this?

Seems like it. We should also then simplify AvroSchema to omit Type.

mtth avatar Sep 21 '24 16:09 mtth

@mtth can't we just add | Type to DefinedType for this?

Seems like it. We should also then simplify AvroSchema to omit Type.

I'll open a PR and give it a short test.

Independently, and I realize this might be contentious: Given there's not a crazy amount of pull requests open where the fallout would be big: what are your thoughts on transforming this project to use Typescript?

joscha avatar Sep 21 '24 16:09 joscha

@mtth can't we just add | Type to DefinedType for this?

Seems like it. We should also then simplify AvroSchema to omit Type.

https://github.com/mtth/avsc/pull/476

joscha avatar Sep 23 '24 10:09 joscha

Given there's not a crazy amount of pull requests open where the fallout would be big: what are your thoughts on transforming this project to use Typescript?

I think it makes sense to switch to TypeScript eventually.

mtth avatar Sep 24 '24 03:09 mtth

I think it makes sense to switch to TypeScript eventually.

okay. Given #452 was just merged, this could be a great time?

joscha avatar Sep 24 '24 07:09 joscha

okay. Given https://github.com/mtth/avsc/pull/452 was just merged, this could be a great time?

If we wanted to include this in 6.0, yes. I'd like to think about it a bit.

mtth avatar Sep 28 '24 18:09 mtth