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

feat(common): support empty @Inject() on constructor-based injection #13428

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Apr 13, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: closes #13426

What is the new behavior?

We can use @Inject() (ie., no token supplied) on constructor-based injections. It works just like property-based injection

Does this PR introduce a breaking change?

  • Yes
  • No

@coveralls
Copy link

Pull Request Test Coverage Report for Build d48faa69-328d-42fb-966e-6301bdeed1a3

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 92.126%

Totals Coverage Status
Change from base Build 011c0093-57c9-44ca-8bbc-3c112693a9e8: 0.003%
Covered Lines: 6739
Relevant Lines: 7315

💛 - Coveralls

Copy link

@benjGam benjGam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi ! I'm not sure to understand, what this will produce as a result ? Will Nest take providers list as ref to auto-fill empty @Inject() calls to avoid errors ?

Edit: Just rephrasing to be clear.

@micalevisk
Copy link
Member Author

micalevisk commented Apr 22, 2024

nestjs will infer the provider token using parameter's metadata. Prior to this, such metadata is always ignored when using Inject on parameters because nestjs always use either the supplied token or the concrete class as the token

I don't know how to explain it more because I didn't follow if you're talking about the transpiled JS code or the runtime behavior.

Here's a video about the reflection on nestjs: https://youtu.be/jo-1EUxMmxc?si=ZvRQ8-6lac6Se3cS&t=779

@benjGam
Copy link

benjGam commented Apr 23, 2024

Sorry, yes i was talking about the runtime behavior.

@micalevisk
Copy link
Member Author

micalevisk commented Apr 23, 2024

regarding that, it will behave just like property-based injection. For them, the @Inject() is used due to introspection limitations. For constructor-based ones, we'll start supporting @Inject() for completeness sake only

@benjGam
Copy link

benjGam commented Apr 23, 2024

Ok, i understand the motivations, thanks for your reply !

@namgk
Copy link

namgk commented Apr 24, 2024

any timeline for this to be merged?

@kamilmysliwiec
Copy link
Member

lgtm

@kamilmysliwiec kamilmysliwiec merged commit 4b07cb2 into nestjs:master Jun 3, 2024
5 checks passed
@micalevisk micalevisk deleted the feat/issue-13426 branch June 3, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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