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

Support using @Inject() decorator factory with no args on constructor-based injections #13426

Open
1 task done
micalevisk opened this issue Apr 12, 2024 · 0 comments · May be fixed by #13428
Open
1 task done

Support using @Inject() decorator factory with no args on constructor-based injections #13426

micalevisk opened this issue Apr 12, 2024 · 0 comments · May be fixed by #13428
Labels
needs triage This issue has not been looked into type: enhancement 🐺

Comments

@micalevisk
Copy link
Member

micalevisk commented Apr 12, 2024

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

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

If we use @Inject() on constructor, Nest will think that that is due to some circular (acting the same as @Inject(undefined), basically). While we should use @Inject() on property-based injections.

For example:

import { Module, Inject } from '@nestjs/common';

class Foo {}

@Module({
  providers: [Foo],
})
export class AppModule {
  @Inject() foo1: Foo

  constructor(
    @Inject(Foo) readonly foo2: Foo,
    @Inject() readonly foo3: Foo, // Fails
  ) {}

  onModuleInit() {
    console.log('foo1 instanceof Foo:', this.foo1 instanceof Foo)
    console.log('foo1 === foo2:', this.foo1 === this.foo2)
    console.log('foo1 === foo3:', this.foo1 === this.foo3)
  }
}

as of now, we'll see the following error:

[Nest] 286235  - 04/12/2024, 12:15:25 PM   ERROR [ExceptionHandler] Nest can't resolve dependencies of the AppModule (Foo, ?). Please make sure that the argument dependency at index [1] is available in the AppModule context.

Potential solutions:
- Is AppModule a valid NestJS module?
- If dependency is a provider, is it part of the current AppModule?
- If dependency is exported from a separate @Module, is that module imported within AppModule?
  @Module({
    imports: [ /* the Module containing dependency */ ]
  })

Describe the solution you'd like

I think that it will be less confusing if we start supporting @Inject() on constructor-based injection as well. Those @Inject() calls just need to be 'ignored', so people won't complain on why they're seen such error. Working the same as not having @Inject()

I managed to change the following code in order to support that:

export function Inject<T = any>(
token?: T,
): PropertyDecorator & ParameterDecorator {
return (target: object, key: string | symbol | undefined, index?: number) => {
const type = token || Reflect.getMetadata('design:type', target, key);

to:

// ...
const injectCallHasArguments = arguments.length > 0
return (target, key, index) => {
	let type = (token || Reflect.getMetadata('design:type', target, key));
    if (!type && !injectCallHasArguments) {
      type = Reflect.getMetadata('design:paramtypes', target, key)?.[index];
    }
// ...

so now I'm seen this output instead of that error:

image

but I'm not sure if that's the right way to implement such feature

Teachability, documentation, adoption, migration strategy

N/A

What is the motivation / use case for changing the behavior?

  • discord questions (from newbies) on why the injecting isn't working as people expect
  • make the property-based injection and constructor-based injection more alike (ie., an uniform API), so we can move from property-based injection to constructor-based and vice-versa without thinking too much. This also should increase the learning curve -- learn once, apply anywhere
@micalevisk micalevisk added type: enhancement 🐺 needs triage This issue has not been looked into labels Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: enhancement 🐺
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant