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.

Sign up for GitHub

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

Jump to bottom

refactor: remove mixins and add AsyncAPI TS types #581

Merged
merged 8 commits into from
Aug 31, 2022
Merged

refactor: remove mixins and add AsyncAPI TS types #581

merged 8 commits into from
Aug 31, 2022

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Aug 26, 2022 •
edited
Loading

Description

  • remove mixins - they create a lot of problems in maintaining and, in addition, do not work well with generic types in TS.
  • add TS interfaces/types for v2 spec - use that interfaces in v2 models

I know that there're a lot of changes, but only due to fact that I had to ride off mixins and add to every model created typings.

cc @smoya

Related issue(s)
Part of #93
Part of #482

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Aug 26, 2022
@magicmatatjahu magicmatatjahu marked this pull request as ready for review August 29, 2022 11:15
@magicmatatjahu magicmatatjahu requested a review from smoya August 29, 2022 11:21
@smoya
Copy link
Member

smoya commented Aug 30, 2022

@magicmatatjahu It was hard to me to understand those interfaces represent the actual AsyncAPI Spec by each version. I want to suggest we change the dir interfaces to something else that helps visualizing what it is actually. Maybe spec or similar?

smoya
smoya reviewed Aug 30, 2022
View reviewed changes
src/interfaces/v2.ts Outdated
@@ -0,0 +1,380 @@
import type { JSONSchema7, JSONSchema7Type } from "json-schema";

export type AsyncAPIVersion = '2.0.0' | '2.1.0' | '2.2.0' | '2.3.0' | '2.4.0';
Copy link
Member

Choose a reason for hiding this comment

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

You can play with @asyncapi/specs package like we do in https://github.com/asyncapi/parser-js/pull/583/files#diff-8fa4b52909f895e8cda060d2035234e0a42ca2c7d3f8f8de1b35a056537bf199R19 instead of hardcoding, so if we release 2.5.0 you don't need to update this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Package @asyncapi/specs hasn't exported packages so we cannot "retrieve" types like here https://github.com/asyncapi/parser-js/pull/583/files#diff-8fa4b52909f895e8cda060d2035234e0a42ca2c7d3f8f8de1b35a056537bf199R19

TS is a something like linter, it doesn't read types from runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also change that type to string.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I would vote for leaving it as string until we refactor @asyncapi/specs to export the types from it (if we do at some point).

Copy link
Member Author

Choose a reason for hiding this comment

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

@smoya Done :)

@magicmatatjahu
Copy link
Member Author

@smoya

@magicmatatjahu It was hard to me to understand those interfaces represent the actual AsyncAPI Spec by each version. I want to suggest we change the dir interfaces to something else that helps visualizing what it is actually. Maybe spec or similar?

Sure, spec-types is probably better, I don't know what spec would be for me if I didn't know about these types 😅

@sonarqubecloud
Copy link

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

@magicmatatjahu magicmatatjahu requested a review from smoya August 31, 2022 10:04
smoya
smoya approved these changes Aug 31, 2022
View reviewed changes
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.

LGTM! 🚀 🌔

@smoya
Copy link
Member

smoya commented Aug 31, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit a40b685 into asyncapi:next-major Aug 31, 2022
@magicmatatjahu magicmatatjahu deleted the next/rideoff-mixins branch August 31, 2022 10:15
magicmatatjahu added a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022
@magicmatatjahu
refactor: remove mixins and add AsyncAPI TS types (asyncapi#581)
033c880
derberg pushed a commit that referenced this pull request Oct 4, 2022
@magicmatatjahu @derberg
refactor: remove mixins and add AsyncAPI TS types (#581)
deb900e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoya smoya smoya approved these changes

@fmvilas fmvilas Awaiting requested review from fmvilas fmvilas is a code owner

@jonaslagoni jonaslagoni Awaiting requested review from jonaslagoni jonaslagoni is a code owner

@derberg derberg Awaiting requested review from derberg derberg is a code owner

@asyncapi-bot-eve asyncapi-bot-eve Awaiting requested review from asyncapi-bot-eve asyncapi-bot-eve is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@magicmatatjahu @smoya @asyncapi-bot