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

Suggestion: Events for Danet #75

Closed
marco-souza opened this issue Feb 7, 2024 · 8 comments · Fixed by #80
Closed

Suggestion: Events for Danet #75

marco-souza opened this issue Feb 7, 2024 · 8 comments · Fixed by #80
Labels
feature New feature

Comments

@marco-souza
Copy link
Contributor

Is your feature request related to a problem? Please describe.

As a Nest.JS developer, I'd like the possibility of making an event-driven system using the framework decorators and module system.

References:

Describe the solution you'd like
I'm not sure about the internals, but I imagine something like this:

this.emitter.connect({
  // Add queue configs when the app is initializing (SQS, Kafka, etc)
})

than, I'd like to bind class methods to events:

// initialization
class MyListeners {

  @OnEvent('new-user')
  handleWelcomeNewUser(user: User) {
    // Do some side effect
  }

}

and also should be able to emit these events, something like:

// initialization
class MyListeners {

  createUser(user: UserDto) {
    // create user
    this.emitter.send('new-user', user)
  }

}

Describe alternatives you've considered

At the moment, the alternative I have considered is to make an EventEmitter module, which manages queues and subscriptions using Deno.Kv queues, to keep it as native as possible.

This is not ideal, but it can work if we set the (un)subscription in onAppBootstrap and onAppClose respectively.

@Injectable()
export class MyListeners implements OnAppBootstrap, OnAppClose {
  listeners: string[] = []

  constructor(@Inject(EventEmitter) private eventEmitter: EventEmitter) {}

  eventHandler() {
    // do something
  }

  async onAppBootstrap() {
    const eventId = await this.eventEmitter.subscribe('event', this.eventHandler);
    this.listeners.push(eventId)
  }

  async onAppClose() {
    await Promise.all(
      this.listeners.map(
        this.eventEmitter.unsubscribe,
      )
    );
  }

}
@marco-souza marco-souza changed the title Suggestion: Event-Driven integrations for Danet Suggestion: Events for Danet Feb 7, 2024
@Sorikairox Sorikairox added the feature New feature label Feb 8, 2024
@Sorikairox
Copy link
Collaborator

Hey @marco-souza, thank you for this Feature Request.

I like it, is it something your would be willing to implement natively in Danet, in a separate module, similar to how the Swagger module work ?

If not, I might have some time to draft something this weekend or next week's !

@marco-souza
Copy link
Contributor Author

@Sorikairox Thank you for the availability.

I was imagining it as a separate module, as you mentioned. I'm interested in working on it, but unfortunately, I won't have the time until the prod release in my job, planned for 1st week of next month.

If you have the time to draft something it would be great! I can help to discuss the proposal, review the code changes, and test it.

@Sorikairox
Copy link
Collaborator

Sorikairox commented Feb 10, 2024

Hey @marco-souza !

I took a look at Nest implementation and your feature request.

There are 2 distinct possible features:

Local event based

Similar to: https://docs.nestjs.com/techniques/events

These are "local" events across the instance, using the EventEmitter2 library, but we now have a Web API that works well too https://developer.mozilla.org/en-US/docs/Web/API/EventTarget

Queue/PubSub

Similar to: https://docs.nestjs.com/techniques/queues

Listening and sending data to a Queue, and we can extrapolate that we can make it work with Pub/Sub too without too much trouble.

From my understanding, you would like the latter so I will start with that with Deno.Kv first (even if both are cool and needed for different use cases).

I have a somewhat clear idea of how to implement that.

@marco-souza
Copy link
Contributor Author

That's great @Sorikairox, thanks!

Regarding the local events, I also find it super handy for different use cases. Today I had some time to draft an Event module simulating the EventEmitter behavior. It's super simple (and suboptimal), so I'd appreciate some feedback.

PR #76

I implemented it as an internal module of danet, as it simplifies event subscription, but I'm unsure if it's the best resolution. I'm also not well-versed in reflect-metadata API (I've heard of it, but today was the first time I used it), so forgive me if my code looks dumb 😆

I'm not sure if it helps the Queue/PubSub feature, but let me know if we can join efforts to have both features 💪🏼

@marco-souza marco-souza mentioned this issue Feb 15, 2024
9 tasks
@Sorikairox
Copy link
Collaborator

@marco-souza Thank you for the PR ! Commenting on it.

I will have time to start working on the Queue/PubSub feature this weekend !

@marco-souza
Copy link
Contributor Author

Hello @Sorikairox 👋

This weekend I was also looking at my personal list of nestjs features that I use most, the next one would be the Task Scheduling feature, for handling batch jobs, clean-up tasks, and other time-related events. It also matches well with the Queue/PubSub feature.

I imagine It would be similar to the Events implementation, and we can use deno native cron jobs with Deno.Cron module. I want to implement it later this week, but first I have a couple of questions:

  1. Should I create a new ticket for tracking this feature?
  2. This feature could be written as a danet_cron external lib, but I also like having these common features out-of-the-box in the framework itself, so the framework would have some kind of standard library. On the other hand, this will increase the framework core size, even for those who won't use any of these features. IMO, we can continue to write these as part of the main framework until it becomes a problem and then we decide how to break it down, but I want to hear your thoughts on this.

@Sorikairox
Copy link
Collaborator

Sorikairox commented Feb 19, 2024

Hey @marco-souza !

  1. Yes !
  2. As long as we create them as we did for EventEmitterModule, we are fine. We will be able to easily migrate them to their own module

I think that tree-shaking is enough to reduce the framework size in bundling/production anyway ? It's a problem for later !

@marco-souza
Copy link
Contributor Author

marco-souza commented Feb 19, 2024

Totally agree with you!
I'll create a new task proposing I created #78 and I'll let you know when I have the PR ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants