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
Domain event emitter V2 #134
Domain event emitter V2 #134
Changes from all commits
7e37e53
a216a24
6a8874d
85d9168
4ba3c03
d730188
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.
this is the most important part to review and criticize
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.
Shouldn't the specific schema extension be part of apps using the library? I understand we want to use this schema but not sure if having it as part of the library is a good idea.
Also, if we want to add/modify a property we will need to release a new version (which is a potential cause of major releases) or create our extension so having our extension from the beginning could be beneficial.
What do you think?
Btw, no strong opinion from my side, I am also fine keeping it here but just raising the topic in case it can be useful
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.
Yes, definitely. Which is why there is an optional extended schema for those who want a reasonable default (e. g. us), but the library itself doesn't rely on it and supports having your own.
If we ever decide to change it, I would expect we would be adding it as a separate schema, e. g.
EXTENDED_MESSAGE_SCHEMA_V2
, so that those who are happy with the original one, can keep using 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.
Yeah, the point was that I am not sure if the library should just define the minimum it needs to work and be open to extensions (it is already) without providing things the users can define based on their use cases, and are potential points of change. Also, to avoid having
EXTENDED_MESSAGE_SCHEMA_V{{N}}
schemas for backward compatibility.I think in our case, I would suggest going with creating our own extension (could be
MY_SCHEMA = EXTENDED_MESSAGE_SCHEMA
) to have flexibility and not have to modify the library to just modify the schemaBut as I mentioned, I am not sure if this concern is valid or if it is stupid, so if you think it is fine, I am also fine and please feel free to resolve this thread if it is the case
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.
Very fair point. But also for standartization sake I would like to have one single place where our common schema is defined, so that each service doesn't have to do it over and over again. It doesn't have to be this library, though, we could put it e. g. in
shared-ts-libs
as a part ofevents-common
.I would still provide EXTENDED_MESSAGE_SCHEMA as an opinionated starting point for people/teams who might benefit from it, but won't couple to it anywhere in message-queue-toolkit itself.
I agree that when we need to adjust our schema, we should be able to do it outside of message-queue-toolkit.