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

feat!: add schema union support #785

Closed

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jun 13, 2023

Description
This PR adds support for schema union support.

Related issue(s)
Related to asyncapi/spec#622
Fixes #809

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jun 13, 2023

@jonaslagoni You shouldn't remove the schemaFormat() method from message object, why? In v2 people still can use that, so they should have possibility to retrieve that value. In v3 we should have still that method, but retrieving logic should be based on the message.payload.schemaFormat value :)

EDIT: Ok, you removed from message trait - but if there's a possibility to have that value in v2 message's trait, the v2 should return that, in v3 we should return undefined.

@jonaslagoni jonaslagoni marked this pull request as ready for review June 16, 2023 11:08
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me in general. Left a minor comment.

Comment on lines +28 to +29
const payloadIsSchemaUnion = this._json.payload.schemaFormat !== undefined;
return payloadIsSchemaUnion ? this._json.payload.schemaFormat : getDefaultSchemaFormat(this._meta.asyncapi.semver.version);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const payloadIsSchemaUnion = this._json.payload.schemaFormat !== undefined;
return payloadIsSchemaUnion ? this._json.payload.schemaFormat : getDefaultSchemaFormat(this._meta.asyncapi.semver.version);
const payloadIsSchemaUnion = this._json.payload?.schemaFormat !== undefined;
return payloadIsSchemaUnion ? this._json.payload.schemaFormat : getDefaultSchemaFormat(this._meta.asyncapi.semver.version);

I think payload can be undefined, right? We should be careful then.

schema: AsyncAPISchemaObject | ReferenceObject | any;
}

export type AllSchemaObjects = MultiFormatSchemaObject | AsyncAPISchemaObject | ReferenceObject | any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me! Just few changes that are needed. Including the one @fmvilas requested in https://github.com/asyncapi/parser-js/pull/785/files#r1242462994

@smoya
Copy link
Member

smoya commented Jul 17, 2023

I'm taking over this one as @jonaslagoni is 🌴 🏖️

@smoya
Copy link
Member

smoya commented Jul 18, 2023

Required Parser-API changes are now in the following PR asyncapi/parser-api#100

cc @jonaslagoni @fmvilas

@sonarcloud
Copy link

sonarcloud bot commented Jul 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@smoya
Copy link
Member

smoya commented Jul 24, 2023

Since this PR has been opened for a while and lot of changes come from next-major-spec, and because I took over the work, I'm gonna close this one and instead open a new PR whenever I have a first working version.

cc @jonaslagoni

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 this pull request may close these issues.

4 participants