-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
docs: small social media system example #651
docs: small social media system example #651
Conversation
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
examples/social-media/readme.md
Outdated
- A frontend application where users interact through the website, where they can like comments. | ||
- A backend WebSocket server that sends and receives events for the UI to update in real-time. | ||
- A comment service which processes all events related to comments through a message broker. | ||
- A public-facing API which allows others to get notified about updates. |
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.
It is not clear enough for me what this public facing API would do. Here you mention allows others to get notified about updates
, however in the public.asyncapi.yaml
file, there is only one operation which is publish
. Shall we add a Subscribe
Operation so users can be notified?
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.
The public API needs to be seen as an external application, that anyone can create to interact with the system. We still use publish
because that application needs to consume messages, because the internal publish to 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.
First great complex example!
Would make sense to adopt https://github.com/amadeus4dev/amadeus-async-flight-status some day too
as for the filenames, can we promote here |
@derberg do you mean switching from |
I only think that this is appropriate if we plan to include implementation as well. Otherwise, I think it makes it less readable, and annoying that they are located in separate directories. |
yeah, my opinion is that people are not so perceptive when learning to notice your example is done this way and not another because there is no code. Imho example should always push best practices. In the real project, this is how it should look like, asyncapi file per folder, not all in one. Also in tools we promote |
That's all good fine until you want to introduce reusability. If we assume each project is a git repository (very likely). If you do That's why I normally place every AsyncAPI document within the same directory/repository because it is more convenient. So about the common file, where we look up stuff, should that be located under |
Last time I did microservices in production, it was indeed a separate repo per project @jonaslagoni
@boyney123 you might want to have a look at this one, it adds more complex structure to examples, not sure how it affects your concept of "starting new file from an existing example" |
Yeah at the moment it just renders things on a 121 basis, but no real reason why this cannot be supported would just need thinking about, or for now we just don't pull it over in the examples when the GitHub action triggers, but yeah would need thinking about. At the moment nothing would break 👍 , so good to think about but don't let it block it |
|
Yeah the idea is to copy over the With the new pattern that would not be the case, so would just need to rethink how that would work, but it's possible. |
title: updateCommentLikesPayload | ||
additionalProperties: false | ||
properties: | ||
commentId: |
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 think a likeCount
should be included as well.
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.
There are definitely multiple things that could be added to make the system more believable. However, we gotta stop somewhere, and I just decided to take a minimal approach.
I guess my question is: would this minimal payload not show what is needed? 🤔
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.
It requires a separate call to update the count, and there are race conditions. I don't feel strongly as it's just an example.
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.
ideal example would be actually something that we can use in code generators to generate real project
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.
@derberg so you also think we should add more details to the messages?
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.
hahaha, when I clicked on Add single comment
I had a quick thought in my head "this one can be confusing" 😄
basically, when I generate code with these examples, in frontend app when I add logic to react on event about comments count, I will not have access to actually the count information. Thus from a code generation perspective, this is not a full example.
Does it make sense?
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, that is a good point...
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.
Alright, I have added a likedBy
property to the likeCommentPayload
, and likeCount
to the two schemas commentChangedPayload
, updateCommentLikesPayload
.
Not sure if likedBy
makes sense 🤔 Maybe better to be left out. What do you think?
Co-authored-by: Chris Eich <ceich62@gmail.com>
Co-authored-by: Chris Eich <ceich62@gmail.com>
@jonaslagoni are you merging this one? |
@derberg right... For some reason, I still have the thought that code owners do the ready for merge because they are satisfied with the state. I guess this lies rather with the PR owners instead then 😄 |
/rtm |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
actually you are right 🤦🏼 I could merge too, I somehow mixed stuff up, / r t m is there to help but still owners can merge 😆 |
/rtm |
@allcontributors please add jonaslagoni for example |
I've put up a pull request to add @jonaslagoni! 🎉 |
🎉 This PR is included in version 2.3.0-2022-01-release.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR introduces the basic social media system that we can use as a baseline for features and improvement discussions as it introduces a lot of different perspectives and how AsyncAPI can be used.
It is based on @fmvilas example in #618, however, adapted to 2.2.0, as there is no need to wait to introduce it.
Fixes #646