refactor: prefer eventService.storeEvent methods #4830
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
| const eventService = new EventService( | ||
| { eventStore, featureTagStore }, | ||
| { getLogger }, | ||
| ); |
There was a problem hiding this comment.
I'd argue that group service should not need to know how to construct an event service, maybe moving event service as a parameter to createGroupService would help with my other comment
There was a problem hiding this comment.
I disagree with this one due to the fact that other createXService methods follow the same approach: They create the needed services inside the methods instead of accepting them as arguments.
If we decide to refactor this I would prefer to target all of those methods and do it in a separate PR.
There was a problem hiding this comment.
I can explain the details behind createXService methods. Those are "composition roots" for each service. So they hide the information how each service is created. Some of our services depend on other services (which is often the consequence of not have read models). If creating a dependent service is difficult I usually create another composition root (createXService) method for the dependent service and call it from my composition root. On the other hand some services are trivial to instantiate or I didn't have time to migrate them to compositon roots. So my recommendation is to decide on a case by case basis. If you feel the pain of constructing a complex graph of objects: create a compositon root for a dependent service.
There was a problem hiding this comment.
they hide the information how each service is created
I think this implies that they should not know how other services are created other than themselves. It acts as a Dependency Injection layer, where services get services injected rather than creating them on each composition root, which might lead to maintainability issues when having to modify how a service is created...
On the other hand some services are trivial to instantiate or I didn't have time to migrate them to composition roots.
I believe this one is simple, but I'm ok with whatever Nuno decides to do here, we'll have a chat and move this forward.
There was a problem hiding this comment.
Also, in other places we prefer injecting the service: https://github.com/Unleash/unleash/pull/4830/files#diff-0f55a0a1460b7ad5fd21da38762c259c1580e877e4b6b746e0032ad27776c886R93 but as Mateusz said, it can be considered tech debt for later
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
nunogois
force-pushed
the
refactor-event-service-store-event
branch
from
c2cbad3
to
f70791f
Compare
https://linear.app/unleash/issue/2-1403/consider-refactoring-the-way-tags-are-fetched-for-the-events
This adds 2 methods to
EventService:storeEvent;storeEvents;This allows us to run event-specific logic inside these methods. In the case of this PR, this means fetching the feature tags in case the event contains a
featureNameand there are no tags specified in the event.This prevents us from having to remember to fetch the tags in order to store feature-related events except for very specific cases, like the deletion of a feature - You can't fetch tags for a feature that no longer exists, so in that case we need to pre-fetch the tags before deleting the feature.
This also allows us to do any event-specific post-processing to the event before reaching the DB layer.
In general I think it's also nicer that we reference the event service instead of the event store directly.
There's a lot of changes and a lot of files touched, but most of it is boilerplate to inject the
eventServicewhere needed instead of using theeventStoredirectly.Hopefully this will be a better approach than #4729