Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: add support for asyncapi 3.0.0 operations and channels #654
chore: add support for asyncapi 3.0.0 operations and channels #654
Changes from 6 commits
0791adc
56b9fd1
1af3ca8
73b009e
5f3c1be
46889bf
7361334
c58a021
aa6cf51
80dd7ea
25e84bd
d900b06
715b97d
9d9c514
cfff064
df73fec
a70e365
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You'll find a bunch of these. Since now a
DetailedAsyncAPI
can be either v2 or v3, we need to make it explicit here.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.
I found some cases in which
pointer
will be undefined and had to do this. Happy to hear about alternatives.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.
In which places? Could you point to them in this PR?
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.
https://github.com/asyncapi/parser-js/pull/654/files#diff-d7ef7ffb6795ee82f8ee52244fb91a58a582e6b65a273d0ec162b0d04ae4c340R71-R73
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.
I changed this because I think it's confusing. An operation shouldn't have an
id
and anoperationId
. TheoperationId
is theid
of the operation 😅 So yeah, changed it and changed tests too.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.
Makes sense 👍 . Would you mind adding such a change into asyncapi/parser-api#71 ?
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.
id
was treated as a key in the collection and not asoperationId
. The fact that in v3operationId
is mandatory (but not for operations in components - here keys are interpreted as a collection's key and notoperationId
), this remains a problem with v2 and how we filter collections -parser-js/src/models/operations.ts
Line 12 in 46889bf
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.
We have to think how to handle it.
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.
I don't think I understand 😅 Just to clarify, in v3
operationId
is not mandatory, it just doesn't exist. The id comes from the key in theoperations
object.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.
from v2 spec point of view it makes sense and that's the problem 😄
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.
I don't think it does, even from v2 point of view.
operationId
was there to mean theid
of the operation. If it's present, it should be the operation identifier. If it's not, ok, we can generate one if you want, but it was never meant to be an "extra" id.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.
Hmm 🤔 I have a problem in general because in the v3 version people will be able to define operations in components and how to distinguish this identifier in
components.operations[x]
fromoperationId
- that's why we have these two methods.id()
method is more of a method used to filter operations by identifier (used as key in thecomponents.operations[x]
andoperations[x]
), but... we'll see how to figure it out andid()
should returnoperationId
. Can we fix it in next PRs? We need also to fix that inparser-api
and removeoperationId()
method.To clarify: I wrote about
components.operations[x]
andoperations[x]
, because inparser-api
we have methodallOperations()
which should return all operations defined in given AsyncAPI document.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.
I realized no one updated the parser-api with any change regarding this.
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.
All is fixed and there is no need to add anything else.
This file was deleted.