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

Domain event emitter V2 #134

Merged
merged 6 commits into from
May 2, 2024
Merged

Domain event emitter V2 #134

merged 6 commits into from
May 2, 2024

Conversation

kibertoad
Copy link
Owner

No description provided.

@kibertoad kibertoad requested review from dariacm and CarlosGamero May 1, 2024 13:10
@kibertoad kibertoad changed the title Follow-up fixes and tweaks for #133 Domain event emitter V2 May 2, 2024
payload: z.optional(z.object({})).describe('event payload based on type'),
})

export const EXTENDED_MESSAGE_SCHEMA = BASE_MESSAGE_SCHEMA.extend({
Copy link
Owner Author

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

Copy link
Collaborator

@CarlosGamero CarlosGamero May 2, 2024

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

Copy link
Owner Author

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?

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.

Copy link
Collaborator

@CarlosGamero CarlosGamero May 2, 2024

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 schema

But 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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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 schema (same problem we have with WS rooms, where to add a room we need a new library version)

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 of events-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.

Copy link
Collaborator

@CarlosGamero CarlosGamero left a comment

Choose a reason for hiding this comment

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

LGTM 🚀! thanks, Igor for doing it and also answering my questions and concerns 🧡

@kibertoad kibertoad merged commit 07e3f3e into main May 2, 2024
5 checks passed
@kibertoad kibertoad deleted the feat/domain-event-emitter branch May 2, 2024 12:58
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