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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Apr 13, 2024 •
edited

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

benjGam
benjGam reviewed Apr 22, 2024
View reviewed changes
Copy link

@benjGam benjGam left a comment •
edited

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 •
edited

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 •
edited

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjGam benjGam benjGam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support using @Inject() decorator factory with no args on constructor-based injections
4 participants
@micalevisk @coveralls @benjGam @namgk