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

Move hook domain logic from routes to domain #1774

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Jul 18, 2024

Summary

With the forthcoming implementation of notification dispatch according to events, hooks will also trigger enqueuing notifications. This moves the domain-considered logic from CacheHooksService to a new HooksRepository in the domain. There the aforementioned notification-relevant logic will be placed.

Changes

  • Move domain logic from CacheHooksService to HooksRepository
  • Propagate changes accordingly

@iamacook iamacook self-assigned this Jul 18, 2024
@iamacook iamacook requested a review from a team as a code owner July 18, 2024 12:10
@iamacook iamacook marked this pull request as draft July 18, 2024 12:12
@iamacook iamacook marked this pull request as ready for review July 18, 2024 12:23
@coveralls
Copy link

coveralls commented Jul 18, 2024

Pull Request Test Coverage Report for Build 10004593065

Details

  • 97 of 102 (95.1%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 48.341%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/cache-hooks/cache-hooks.service.ts 4 5 80.0%
src/domain/hooks/hooks.repository.ts 79 83 95.18%
Totals Coverage Status
Change from base Build 9991351602: 0.1%
Covered Lines: 4288
Relevant Lines: 7154

💛 - Coveralls

this.queueName = this.configurationService.getOrThrow<string>('amqp.queue');
}

onModuleInit(): Promise<void> {
Copy link
Member

@hectorgomezv hectorgomezv Jul 18, 2024

Choose a reason for hiding this comment

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

I think we might face a race condition here: if this onModuleInit function is executed before CacheHooksService.onModuleinit, then this.listeners would be empty, and therefore the first events wouldn't be processed by any listener, right? I don't know if they would be lost as they wouldn't be ACKed but maybe the events processing order is modified. I'd avoid relying on the class instantiation order, but maybe I'm missing something.

What do you think about moving the complete CacheHooksService implementation to the domain layer instead (including its onEvent function). To me, it was always a bit weird that this core logic lives in the routes layer so this might be a good opportunity to move it to its own class in the domain and would save us a level of indirection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good points, with which I agree with. As of 6271732, I've moved the relevant CacheHooksService logic to the domain. Please let me know what you think.

@iamacook iamacook marked this pull request as draft July 19, 2024 07:23
@iamacook iamacook marked this pull request as ready for review July 19, 2024 07:45
@iamacook iamacook changed the title Accept multiple listeners for the same queue subscription Move hook domain logic from routes to domain Jul 19, 2024
Copy link
Member

@hectorgomezv hectorgomezv left a comment

Choose a reason for hiding this comment

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

🚀

@iamacook iamacook merged commit 53b7964 into main Jul 19, 2024
16 checks passed
@iamacook iamacook deleted the multiple-queue-subscriptions branch July 19, 2024 09:52
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