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

Using seprately declared enum in union in record. #461

Closed
svirpridon opened this issue May 27, 2024 · 8 comments · Fixed by #476
Closed

Using seprately declared enum in union in record. #461

svirpridon opened this issue May 27, 2024 · 8 comments · Fixed by #476

Comments

@svirpridon
Copy link

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!

@mtth
Copy link
Owner

mtth commented Jun 21, 2024

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}});

@joscha
Copy link
Contributor

joscha commented Sep 19, 2024

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

@mtth
Copy link
Owner

mtth commented Sep 21, 2024

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

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

@joscha
Copy link
Contributor

joscha commented Sep 21, 2024

@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 joscha mentioned this issue Sep 23, 2024
@joscha
Copy link
Contributor

joscha commented Sep 23, 2024

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

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

#476

@mtth
Copy link
Owner

mtth commented Sep 24, 2024

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.

@joscha
Copy link
Contributor

joscha commented Sep 24, 2024

I think it makes sense to switch to TypeScript eventually.

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

@mtth
Copy link
Owner

mtth commented Sep 28, 2024

okay. Given #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.

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

Successfully merging a pull request may close this issue.

3 participants