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

fix: importsNotUsedAsValues: remove breaks DI #778

Open
mt3o opened this issue Jun 6, 2022 · 3 comments
Open

fix: importsNotUsedAsValues: remove breaks DI #778

mt3o opened this issue Jun 6, 2022 · 3 comments
Labels
status: needs triage Issues which needs to be reproduced to be verified report. type: fix Issues describing a broken feature.

Comments

@mt3o
Copy link

mt3o commented Jun 6, 2022

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 mt3o added status: needs triage Issues which needs to be reproduced to be verified report. type: fix Issues describing a broken feature. labels Jun 6, 2022
@freshgum-bubbles
Copy link

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.

@mt3o
Copy link
Author

mt3o commented Jun 27, 2023

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.

@freshgum-bubbles
Copy link

freshgum-bubbles commented Jun 27, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs triage Issues which needs to be reproduced to be verified report. type: fix Issues describing a broken feature.
Development

No branches or pull requests

2 participants