feat(mikro-orm): manage lifecycle of subscribers

[derevnjuk]

Information

Type	Breaking change
Feature	No

---

Usage example

Allow managing the lifecycle of subscribers using the IoC container.

To automatically resolve a subscriber's dependencies, you can use the @Subscriber decorator as follows:

import {EventSubscriber} from "@mikro-orm/core";
import {Subscriber} from "@tsed/mikro-orm";

@Subscriber()
export class SomeSubscriber implements EventSubscriber {
  // ...
}

In this example, we register the SomeSubscriber subscriber, which is automatically instantiated by the module using the IoC container, allowing you to easily manage the dependencies of your subscribers.

You can also specify the context name for a subscriber to tie it to a particular instance of the ORM:

import {EventSubscriber} from "@mikro-orm/core";
import {Subscriber} from "@tsed/mikro-orm";

@Subscriber({contextName: "mongodb"})
export class SomeSubscriber implements EventSubscriber {
  // ...
}

Todos

• Tests
• Coverage
• Example
• Documentation

closes #2292

[Romakita]

Hello @derevnjuk

Nice PR. But why a Subscriber isn't decorated by @Injectable decorator ?

Support both ComplexSubscriber and SimpleSubscriber seems to be confusing. Especially the SimpleSubscriber, the developer won't understand why the class isn't injectable and cannot inject something from the DI.

You can simplify the code by introducing a @Subscriber() decorator, and the decorator will collect all complex subscribers automatically.

Decorators:

const MIKRO_ORM_SUBSCRIBERS = "mikro:orm:subscribers";

function Subscriber(opts: {contextName: string} = {contextName:"default"}) {
    return useDecorators(Injectable({type: MIKRO_ORM_SUBSCRIBERS }), StoreSet(MIKRO_ORM_SUBSCRIBERS, opts))
}

The module:

export class MikroOrmModule implements OnDestroy, OnInit, AlterRunInContext {
    constructor(@Inject(MIKRO_ORM_SUBSCRIBERS) private subscribers: EventSubscriber[]) {
    
    }
    
    $onInit() {
       this.settings.map((opts) => {
           this.registry.register({
             ...opts, 
             subscribers: [
                ...opts.subscribers || [], // if you want to maintain simple subscribers
                ...this.getSubscribers(opts.contextName || 'default') // injectable subscribers by contextName
             ]
          })
       })
    }

    getSubscribers(contextName: string ){
      return this.subscribers.filter((subscriberInstance) => {
         return Store.from(classOf(subscriberInstance)).get(MIKRO_ORM_SUBSCRIBERS)?.contextName === contextName)
      })
   }
}

Usage:

@Subscriber()
export class ComplexSubscriber implements EventSubscriber {
  @Inject() private readonly logger: Logger

  public async afterFlush(_: TransactionEventArgs): Promise<void> {
    this.logger.info("Changes has been flushed.");
  }
}

[derevnjuk]

Thank you, @Romakita, for the detailed feedback 👍🏾

Support both ComplexSubscriber and SimpleSubscriber seems to be confusing.

I wanted to demonstrate that it's still possible to pass an instance of a class in the same way it's done by default in Mikro-Orm. This is just a showcase to highlight the preservation of backward compatibility.

You can simplify the code by introducing a @Subscriber() decorator, and the decorator will collect all complex subscribers automatically.

This is exactly what I wanted to avoid 😄 If you believe it would be more appropriate, I'll make these changes

[Romakita]

You can simplify the code by introducing a @subscriber() decorator, and the decorator will collect all complex subscribers automatically.

This is exactly what I wanted to avoid 😄 If you believe it would be more appropriate, I'll make these changes

By principle, all injectable class must be decorated by a decorator (Injectable or similar things). Introducing exception may be confusing for me when an issue will occurs in future. In fact, you are just lucky that the Ts.ED DI works without by using lazyInvoke (but I wasn't his first purpose to allow that - you just made me discover it 🤣).

I understand your point. But the Ts.ED philosophy is to have less configuration has is possible and let the DI discover the classes and configure the third party framework automatically (like his is with the decorator Subscriber suggestion).

One that it could be important to set the classes on subscribers options is the order declaration.
I don't know if the subscribers order is important or not. But if is it, my actual suggestion, won't be a good solution. Like the mount option, the controller declaration order is really import, and could the same problem for subscribers.

I let you give me an answer, and I would reconsider my position ^^.

See you
Romain

[derevnjuk]

@Romakita, this is because MikroOrm already has its own @Subscriber decorator (https://mikro-orm.io/docs/decorators#subscriber). The main aim was to prevent conflicts between symbols and minimize the chances of confusion. However, it is not the best solution, I agree. Would you prefer to keep the same name (i.e. @Subscriber) or choose a new one?

[Romakita]

Hum ok it’s clear. Having twice Subscriber decorator will be confusing. You’ve right.

But the if the mikro-orm exists it means the complex subscriber should always be used to declare Subscriber.

Ok I’ll try to give you appropriate answer tomorrow ;)

[Romakita]

Hum well you have done all changes but your last arguments need to rework again the PR.

I inspected the mikro orm code. The Subscriber decorator create directly the class instance and add it to the subscribers registry:

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/decorators/Subscriber.ts

In fact this scenario must be take in count. Subscribers instance should be injectable to another service isn't supposed to be possible. By proposal, to introduce a Subscriber is wrong (collision decorator name and not aligned with Mikro ORM implementation).

I pushed a commit to rollback Subscriber decorator and allow any subscriber to inject service without adding it to the Ts.ED DI registry ;)

Tell if it's fine for you
Romain

[derevnjuk]

@Romakita looks good, but it may be worth considering avoiding passing a class to subscribers since the @Subscriber decorator and injectable properties already cover this functionality:

const subscribers = opts.subscribers.map((subscriber) => {
-  if (isFunction(subscriber)) {
-    return this.injector.invoke(subscriber, container, diOpts);
-  }
-
  this.injector.bindInjectableProperties(instance, container, diOpts);

  return subscriber;
});

For instance:

@Subscriber()
class EventSubscriber1 implmenets EventSubscriber {
  @Inject()
  private readonly logger!: Logger;
}

class EventSubscriber2 implmenets EventSubscriber {
  @Inject()
  private readonly logger!: Logger;
}

// ...

mikroOrm: [
  {
     subscribers: [new EventSubscriber2()]
  }
]

If you are ok with it, I will update the implementation and docs accordingly.

[Romakita]

@derevnjuk Hum we are not supposed to make new EventSubscriber2. I’m not sure if it’s normal and what is the interest for the developer?

DI is supposed to avoid the manual class construction.

see you

[Romakita]

@derevnjuk I'm sorry I totally forgot this PR.
Is there something I can do to merge this PR ?

[derevnjuk]

@Romakita I apologize for the delay as I'm currently occupied with other tasks. However, I'm aiming to finalize everything this week. BTW seems mikro-orm is going to remove the @Subscriber decorator: mikro-orm/mikro-orm#4231. We can revisit this approach)

[derevnjuk]

@Romakita, I have completed the PR and made a few changes since the last review:

• reverted the @Subscriber decorator back, the mikro-orm is going to remove it from their side for simplicity: Remove @Subscriber() decorator mikro-orm/mikro-orm#4231.
• left the support for both class references and instances in the subscribers option to ensure backward compatibility.

[Romakita]

Seems to be good for me ;) Ready to merge. It's for you OK

@derevnjuk thanks again for this PR!

[derevnjuk]

@Romakita thanks, let's merge 😄

[Romakita]

🎉 This PR is included in version 7.32.0 🎉

The release is available on:

• v7.32.0
• GitHub release

Your semantic-release bot 📦🚀

[derevnjuk]

@Romakita it appears that you forgot to squash commits before merging to the production branch 😢