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

asyncapi 1.0.0 support. #175

Closed
wants to merge 9 commits into from

Conversation

Hexenon
Copy link

@Hexenon Hexenon commented Oct 12, 2019

Added support for asyncapi 1.0.0 1.0.0...

code-first tool to generate asyncapi 1.0.0 docs

recommended to use it with widdershins.

@Hexenon Hexenon mentioned this pull request Oct 12, 2019
@kalinchernev
Copy link
Contributor

Hi @Hexenon thanks so much for the suggestion!

I haven't used asyncapi and don't know much about it. Having a brief look at the project page, it seems like it's a spec similar to open api / swagger, but for event-driven projects. It's interesting!

Not knowing much about this asyncapi I'll need to spend some time to understand the implications if I'm to merge changes in the master. I reallly appreciate the fact you've made the suggestion with tests! My question (not knowing the asyncapi) is: are these 2 specs mutually exclusive or is it possible to combine the usage of both in a single project? I mean, is there an overlap?

Having a quick look at the code changes, I think this part https://github.com/Surnet/swagger-jsdoc/pull/175/files#diff-e3355e060024e166ed7cd8f78dcdb9a4R23-R30 would be easier to understand in a switch statement. It's also the v3 being something related to open api v3 which i'm not sure whether is actually matching to the model of asyncapi.

Next, if we are to have 2 functions to clean unnecessary properties, such as the cleanUselessPropertiesAsyncApi, it'd be actually better to think about a more generic approach of what properties we are interested in in a spec, rather than excluding them. I mean, it's easier to understand and follow a logic of having list a with props acceptable for usage of open api, and list b with acceptable properties of asyncapi. Then, depending on the spec a given code block is aiming to annotate, end result checks for target match of desired values.

If two specs can be used in one project, we'll have to plan for a way to not only organize the parsing of 2, but the output of 2.

I'm sharing the thinking process, because if it happens that the 2 specs are not very compatible and will be rare to have them in a single project, then it might be better to take some utilities of swagger-jsdoc and create a new project for this specific spec.

I've shared some of the thinking process of making this library more generic of moving to factory function deciding upon a parser per annotation type: #65

@chdanielmueller @drGrove @eljefedelrodeodeljefe any thoughts on this?

@Hexenon
Copy link
Author

Hexenon commented Oct 14, 2019

@kalinchernev yeah indeed there are some possible improvements to do. I Agree with the refactor, its almost 100% generic, but it may need some time to invest (which i don't have right now)

Ill be glad if you review what i did and guide me to improve it.

Thanks!

@eljefedelrodeodeljefe
Copy link

eljefedelrodeodeljefe commented May 7, 2020

Frankly the direction of the PR is not what I would expect. Swagger and asyncapi relate only in their syntax and willingness to be compatible to each other, but do not share a common common protocol.

The reason I would take this feature and am currently interested in it, is a HTTP likely being also a broker and sharing e.g. datatbase resources. Hence, an object in swagger will likely also be one in async api, which makes swagger-jsdoc an extreme sharp tool covering both.

Just thinking out loud, I would do:

  • introduce a tag @asyncapi that will be parsed and can receive components.
  • add an asyncapi definition to the options
  • have one outlet for openapi and asnyc api (unlike what we offer right now)
  • possibly use the asyncapi-parser instead
  • separate asyncapi from openapi due to future feature imparity
  • not introduce lodash eagerly

specification[property] = specification[property] || {};
});
v3.baseTopic = '';
} else if (specification.openapi) {

Choose a reason for hiding this comment

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

I'd take that and refactor it after the PR

@@ -18,9 +18,16 @@ function createSpecification(definition) {
'parameters',
'securityDefinitions',
];
const v3 = [...v2, 'components'];
let v3 = [...v2, 'components'];

Choose a reason for hiding this comment

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

as noted in the comments, do not eagerly share the parser. Also this mutation is not very nice

@@ -1,4 +1,5 @@
const parser = require('swagger-parser');
const { isEmpty } = require('lodash');

Choose a reason for hiding this comment

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

if lodash only import that particular function into context. Rather use something like just-is-empty

* @param {object} inputSpec - The asyncapi specification
* @param {object} improvedSpec - The cleaned version of the inputSpec
*/
function cleanUselessPropertiesAsyncApi(inputSpec) {

Choose a reason for hiding this comment

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

As noted in the comments, do not eagerly share parser and then move things back and forth

if (specification.asyncapi) {
specification = cleanUselessPropertiesAsyncApi(specification);
if (isEmpty(specification.baseTopic)) {
delete specification.baseTopic;

Choose a reason for hiding this comment

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

do not delete in JS

swaggerObject[keyName][definitionName],
pathObject[propertyName][definitionName]
);
}

Choose a reason for hiding this comment

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

seems a bit a too central change and an eager refactoring, due to sharing the parser

@Hexenon
Copy link
Author

Hexenon commented May 7, 2020

Frankly the direction of the PR is not what would I expect. Swagger and asyncapi relate only in their syntax and willingness to be compatible to each other, but do not share a common common protocol.

The reason I would take this feature and am currently interested in it, is a HTTP likely being also a broker and sharing e.g. datatbase resources. Hence, an object in swagger will likely also be one in async api, which makes swagger-jsdoc an extreme sharp tool covering both.

Just thinking out loud, I would do:

  • introduce a tag @asyncapi that will be parsed and can receive components.
  • add an asyncapi definition to the options
  • have one outlet for openapi and asnyc api (unlike what we offer right now)
  • possibly use the asyncapi-parser instead
  • separate asyncapi from openapi due to future feature imparity
  • not introduce lodash eagerly
  • start off with asyncapi 2.0.0

Thanks, ill retake when i have time, i started on asyncapi 1.0.0 because of widdershins(only support 1.0.0) and it was a requirement on our company. We are using this forked version.

@eljefedelrodeodeljefe
Copy link

Hi @Hexenon thanks so much for the suggestion!

I haven't used asyncapi and don't know much about it. Having a brief look at the project page, it seems like it's a spec similar to open api / swagger, but for event-driven projects. It's interesting!

Not knowing much about this asyncapi I'll need to spend some time to understand the implications if I'm to merge changes in the master. I reallly appreciate the fact you've made the suggestion with tests! My question (not knowing the asyncapi) is: are these 2 specs mutually exclusive or is it possible to combine the usage of both in a single project? I mean, is there an overlap?

Having a quick look at the code changes, I think this part https://github.com/Surnet/swagger-jsdoc/pull/175/files#diff-e3355e060024e166ed7cd8f78dcdb9a4R23-R30 would be easier to understand in a switch statement. It's also the v3 being something related to open api v3 which i'm not sure whether is actually matching to the model of asyncapi.

Next, if we are to have 2 functions to clean unnecessary properties, such as the cleanUselessPropertiesAsyncApi, it'd be actually better to think about a more generic approach of what properties we are interested in in a spec, rather than excluding them. I mean, it's easier to understand and follow a logic of having list a with props acceptable for usage of open api, and list b with acceptable properties of asyncapi. Then, depending on the spec a given code block is aiming to annotate, end result checks for target match of desired values.

If two specs can be used in one project, we'll have to plan for a way to not only organize the parsing of 2, but the output of 2.

I'm sharing the thinking process, because if it happens that the 2 specs are not very compatible and will be rare to have them in a single project, then it might be better to take some utilities of swagger-jsdoc and create a new project for this specific spec.

I've shared some of the thinking process of making this library more generic of moving to factory function deciding upon a parser per annotation type: #65

@chdanielmueller @drGrove @eljefedelrodeodeljefe any thoughts on this?

sorry I repeated a bit your points :)

Also I will think about it the coming week as I have need for such a feature in a project. It would be nice if we could think about the if and later API and implementation for this feature.

@kalinchernev
Copy link
Contributor

Please see #174 (comment)

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.

3 participants