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

refactor: interface and implementations for each major spec version #488

Merged
merged 5 commits into from
Mar 11, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Mar 9, 2022

Description

The new Intent-driven API is made for keeping retro compatibility and also avoiding breaking changes as much as we can.
This means we should use the same API interface, but keep implementation logic that can differ between different AsyncAPI Spec versions.

This POC is just one suggestion on how we can handle that in the most simple way, avoiding error-prone solutions, overengineered and/or unmaintainable code.

My suggestion is about creating interfaces (actually TypeScript Interfaces) for all the models required for implementing the Intent-Driven API (See https://github.com/asyncapi/parser-api/blob/master/docs/v1.md).
Then, implementation classes per each major AsyncAPI Version, e.g. v2, v3, etc.

The code changed on this PR tries to illustrate that, having the interfaces located at src/models and all implementations under a directory, e.g. src/models/v2 and src/models/v3, meaning v2.x and v3.x spec major versions respectively.

For using one model or the other, we would just need to implement a factory pattern in a function that, based on the version of the provided AsyncAPI document when calling the parse method, will instantiate one or another model. That's all since the interface will be always the same, the only thing that changes is the implementation.

PROS

  • The code is cleaner, all details of implementation are inside each major implementation, no need to do any arcane magic checking versions.
  • Less error-prone, all implementations can be tested with a very robust test suite.
  • Code is isolated, a bug in one major version implementation won't affect others.
  • Scalable, just copy-paste one version and change all you need.

CONS

  • Code is duplicated, meaning that the codebase will increase (on each AsyncAPI spec major version).

I would like to know WDYT about it. cc @magicmatatjahu @Souvikns @jonaslagoni

Related issue(s)

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

I see places to improve :)

src/models/v3/base.ts Outdated Show resolved Hide resolved
Comment on lines 23 to 31
contact(): V2Contact | undefined {
const doc = this.json("contact");
return doc && new V2Contact(doc);
}

license(): V2License | undefined {
const doc = this.json("license");
return doc && new V2License(doc);
}
Copy link
Member

Choose a reason for hiding this comment

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

contact() should return ContactInterface not V2Contact etc

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems it is not necessary to do that since the returned type already implements the interface.
I'm coming from Go land, where it is standard to return the implementation types rather than the interface ones (even though from my point of view, I could find pros and cons in all ways).
I couldn't find examples of this in TS besides a few. I'm happy to change it if you consider it should be done.

@Souvikns
Copy link
Member

Souvikns commented Mar 9, 2022

What I understand for spec version v3 parser will use V3AsyncAPIDocument, but with a newer spec release if we update the models(the interface) then the older parser will also have to implement that change, and if it does will the old parser face any issue.

@magicmatatjahu
Copy link
Member

I'm not a fan of calling things v2 v3 etc. of course we can and should have aliases, but the classes themselves can be called License, Contact etc without those prefixes. If we import or export them then we should use prefixes (but i prefer suffixes), but for models of the same version we should use normal names. Example implementation:

// mdoels/v2/contact.ts
export class Contact extends BaseModel implements ContactInterface {
  ...
} 

export { Contact as ContactV2 }

// models/v2/index.ts
export { ContactV2 } from './contact';

// models/index.ts
export * from './v2';

and of course please write single factory to show how we should handle different classes across versions. I more or less know, but this will help other people :)

That's all I have so far and I don't see any other problems. I like the idea and if something will be wrong, we will change it in the code :)

@smoya
Copy link
Member Author

smoya commented Mar 9, 2022

and of course please write single factory to show how we should handle different classes across versions. I more or less know, but this will help other people :)

This is a thing now

@smoya
Copy link
Member Author

smoya commented Mar 9, 2022

What I understand for spec version v3 parser will use V3AsyncAPIDocument, but with a newer spec release if we update the models(the interface) then the older parser will also have to implement that change, and if it does will the old parser face any issue.

The Interface will not change as far as https://github.com/asyncapi/parser-api/blob/master/docs/v1.md doesn't change.
Of course it will evolve in the future and we will need to update the interface and implementations. At some point, we will drop support for old versions as we are currently doing (e.g. we do not support v1 spec version).

@smoya smoya changed the title poc: interface and implementations for each major spec version refactor: interface and implementations for each major spec version Mar 9, 2022
@smoya smoya marked this pull request as ready for review March 9, 2022 19:38
@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2022

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 Author

smoya commented Mar 10, 2022

I'm not a fan of calling things v2 v3 etc. of course we can and should have aliases, but the classes themselves can be called License, Contact etc without those prefixes.

I finally implemented this change as it also will help reusing code later on.

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Awesome! I don't see any problems, now 😆 Of course the errors themselves (their description) need to be improved, but it's not the most important thing right now :)

🚀

@smoya
Copy link
Member Author

smoya commented Mar 11, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 9769ac4 into asyncapi:next-major Mar 11, 2022
@smoya smoya deleted the compatibility branch March 11, 2022 10:46
magicmatatjahu pushed a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants