-
Notifications
You must be signed in to change notification settings - Fork 144
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
WIP: Enabling forValue to generate named record types #258
base: master
Are you sure you want to change the base?
WIP: Enabling forValue to generate named record types #258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @pleszczy. This is a nice idea but we will need a more involved change to support it (see inline comment).
PS: I updated the title of the PR to be more accurate since forValue
already parses TypeScript objects, generating anonymous types for them. Let me know if I'm misunderstanding.
return Type.forSchema({ | ||
type: 'record', | ||
name: valueType !== Object.constructor.name ? valueType : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work unfortunately: inferring a type from any value which reuses a constructor will fail (a type with the same name will be defined twice). For example, this will often happen when inferring array types.
class Foo {
constructor(val) { this.value = val; }
}
const type = avro.Type.forValue([new Foo(1), new Foo(2)]); // Will throw.
One option could be to add a unique ID inside each name (e.g. generating Foo.g1
, Foo.g2
, ...), update combineObjects
to use it, then do a final pass on the outermost type to combine all generated types from the same constructor and strip the ID.
For backwards compatibility, we should also guard it with an option, say useConstructorNames
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello mtth,
Sorry about that PR, I was just experimenting on my fork and didn't mean to make it towards the upstream master. Still thank you very much for the input, that s a pretty good idea.
No description provided.