typedi icon indicating copy to clipboard operation
typedi copied to clipboard

fix: `importsNotUsedAsValues: remove` breaks DI

Open mt3o opened this issue 3 years ago • 3 comments

Description

Hi!

I think you should expand your documentation by adding a point about enabling "importsNotUsedAsValues": "preserve" in tsconfig.json.

By default typescript removes unused imports, so if class is not imported & instantiated by hand, the call gets removed, and as effect, typedi is unaware of class existence. Enabling the mentioned rule changes the behavior - and explicit import saves the day.

People coming from other languages (java, dot net) or even python might be at difficulty with this behavior. Usually the DI container scans the codebase and detects all classes annotated with @Service or other annotation. Typedi on the other hand - doesn't. If the code is not explicitly executed, class can't be injected.

This pattern is useful when we split interface/abstract class from the implementation/concrete class, and put them in separate files. But not with typedi. Interface gets imported, implementation is not, typedi can't inject. Your tests involving inheritance work because everything stays in the same file. If you put classes into separate files - it will not work.

I made an abstracted sample: (https://github.com/mt3o/typedi-caveat)

Expected behavior

Ideal solution would be to autoscan classes. Good enough would be to instruct the developer to change tsconfig.json and explicitly import all classes.

mt3o avatar Jun 06 '22 13:06 mt3o

As an interested bystander who knows a bit about how this library works:

How would you reasonably auto-scan classes though? Iterating the file-system adds a LOT of complexity and, considering this is also usable on the web, doesn't really seem feasible.

If you're not happy with how TypeDI does it, there's always the injection-js approach: declare every service in one file. Of course, that array can quickly grow into 100L+ in a larger project, making it quite annoying to maintain.

freshgum-bubbles avatar Jun 25 '23 03:06 freshgum-bubbles

I think that acceptable solution would be to explicitly mention that tsconfig.json must be updated so that unused imports are kept, and user is informed that classes must be imported, and that best practice is to keep single file importing all DI-ed classes.

Ideal solution would be to scan files during compilation, and prepare such "load all classes" file. After all, the library is focused on typescript, not pure javascript, and all ts must be compiled to js, so having a typescript compiler plugin should help here.

mt3o avatar Jun 27 '23 08:06 mt3o

I think that acceptable solution would be to explicitly mention that tsconfig.json must be updated so that unused imports are kept, and user is informed that classes must be imported, and that best practice is to keep single file importing all DI-ed classes.

Sounds quite sensible. I'd say send a PR, but this library seems to be abandoned. I think this is generally good advice though, so I'll include it in my TypeDI fork (if you're interested, read more here :-D): https://github.com/freshgum-bubbles/typedi.

Ideal solution would be to scan files during compilation, and prepare such "load all classes" file. After all, the library is focused on typescript, not pure javascript, and all ts must be compiled to js, so having a typescript compiler plugin should help here.

Yes, you could always go the way of Dagger and create a statically-compiled DI system that hard-codes object instantiation. Then again, it'd have to store instances somewhere, so we're basically just re-creating a more primitive ContainerInstance implementation. Although I'll admit I'd like to experiment more in this area.

freshgum-bubbles avatar Jun 27 '23 10:06 freshgum-bubbles