discordx-ts / discordx

🤖 Create a discord bot with TypeScript and Decorators!
https://discordx.js.org
Apache License 2.0
584 stars 50 forks source link

refactor(di): added clearAllServices method for engine and updated DIService to act as bridge for engine only #966

Closed samarmeena closed 7 months ago

samarmeena commented 7 months ago

Engines (A briefing)

They store singleton instances of class.

What's added?

A engine method, that allow cleanup for supported dependency engines.

VictoriqueMoe commented 7 months ago

closing this PR due to architecture breakage and it being stupid

Satont commented 7 months ago

Please not.

Sharlottes commented 7 months ago

Example: How to use TSyringe engine instead of default engine.

You don't have to break the singleton to use an engine other than the default one; I think it's possible to do it within the singleton structure.

As most of the users, use default engine and other engines are optional, it's better to exclude them from dependency as well.

but still in dev deps 🤔 While there are benefits to reducing bundle size at the production level, I'm not sure that's truly the purpose of this PR.

VictoriqueMoe commented 7 months ago

I will give you many reasons as to why this is a bad PR and should not be merged or even used.

first why? why are you doing this. this is not a cleanup. this is how DI works, DI works on the underlying notion of a singleton. the engines are proxies to allow agnostic interaction with underlying DI engines.

If you suddently make an engine a non-singleton, you are making people not only create instances of multuple containers. but also breaking the conteract btetween the framework and the exposed engine layer.

these classes should NOT be constructed. why do you think, in for example, typeDI and tsyringe conainer is a static singleton ?

Do you do new Container? no. because that is bad.

you are also totally breaking TypeDI by allowing any token to be used on the constructor (for some reason).

TypeDI, if you want to use tokens, needs to be a Token of the typeDI framework. not an unknown

Plus, why did you put token on the typeDI consturctor? but use setters for everything else?

If you create 2 instances of any of the engines, you have broken it. this PR should not be merged, i spent a LOT of time coming up with an abstract agnostic notion of allowing people to plugin in multiple and any DI framework they want, and you come along and totally break it. but WHAT REASON? give a good reason. "clean up" is a terrible reason.

samarmeena commented 7 months ago

thank you all for taking time to review and sharing your feedbacks. I have reverted most of the changed and only added a new method (clearAllServices) for supporting engines.

DIService is removed as instance, and only serve as bridge to engine. therefore deprecated instance getter. see below diff.

- DIService.instance
+ Diservice.engine

both properties already exist since long time, but deperacting DIService.instnace from now on. And will be removed in future.

Thank you

VictoriqueMoe commented 7 months ago

after re-evaluating the latest change. it might not seem obvious as to what the acual point of this PR or the change is.

fundimentally, a new method has been added to the DI interface clearAllServices that is meant to clear all the registered singletons in the selected DI service.

A whole re-write was not needed. and this simple change is fine