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

feature(core): Add multi option for custom providers #1517

Closed

Conversation

BrunnerLivio
Copy link
Member

@BrunnerLivio BrunnerLivio commented Feb 2, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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?

No multi provider option

Issue Number: #770

What is the new behavior?

Support the multi option for providers

@Module({
  providers: [{
    provide: 'TEST',
    useValue: 'a',
    multi: true,
  },
  {
    provide: 'TEST',
    useValue: 'b',
    multi: true,
  }],
})
export class ApplicationModule { }

async function bootstrap() {
  const app = await NestFactory.createApplicationContext(ApplicationModule);
  console.log(app.get('TEST')); // ['a', 'b']
}
bootstrap();

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I am looking forward to implementing a APP_INITIALIZER token for NestJS which could help with #1438

@BrunnerLivio BrunnerLivio changed the title [WIP] feature(core): Add multi option for custom useValue-providers [WIP] feature(core): Add multi option for custom providers Feb 2, 2019
@BrunnerLivio BrunnerLivio force-pushed the feature/multi-provider branch 5 times, most recently from 5062a90 to ac78d6e Compare February 3, 2019 15:45
@coveralls
Copy link

coveralls commented Feb 3, 2019

Pull Request Test Coverage Report for Build 1484

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

Totals Coverage Status
Change from base Build 1414: 0.04%
Covered Lines: 3263
Relevant Lines: 3409

💛 - Coveralls

@BrunnerLivio BrunnerLivio changed the title [WIP] feature(core): Add multi option for custom providers feature(core): Add multi option for custom providers Feb 3, 2019
@BrunnerLivio
Copy link
Member Author

BrunnerLivio commented Feb 3, 2019

Done! Please review @kamilmysliwiec

How does it work?

I tried to mimic the behavior of Angular.
When using the multi: true it creates two providers:

  • The actual provider, same as before, but the token is different because the token willl be used for the multi provider.

  • The multi provider is a factory which injects all the actual providers with the same token and returns them as an array

Mimic of the PRs behavior using the current public API to understand what is actually happening

@Module({
  providers: [
    {
      provide: 'TEST', // token gets changed in the background to `{ provide: 'TEST', useValue: 'test2', multi: true}`
      useValue: 'test1',
      multi: true,
    },
    {
      provide: 'TEST', // token gets changed in the background to `{ provide: 'TEST', useValue: 'test1', multi: true}`
      useValue: 'test1',
      multi: true
    }
    {
      // The multi provider
      provide: 'TEST',
      useFactory: (...args) => args,
      // Inject the previous defined providers
      inject: [{provide: 'TEST', useValue: 'test1', ... }, {provide: 'TEST', useValue: 'test2', ...}]
  ],
})
class TestModule { }

@thesayyn
Copy link

Waiting for this. I will migrate my backend to nestjs

@BrunnerLivio
Copy link
Member Author

@thesayyn I am currently stuck because the current PR does not take the export of providers in consideration, which is definitely mandatory. I am currently trying to find a solution for this, but it is not easy, also because Angular behaves differently and therefore I can not proceed with my current Angular-inspired PR :/

@thesayyn
Copy link

thesayyn commented Mar 4, 2019

Let me think a while I may can find a solution

@zuohuadong
Copy link
Contributor

Is there any progress?

let inject: any[] = null;
const multiProviderToken = provider.provide;

token = provider;

Choose a reason for hiding this comment

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

This is my first time looking at nestjs source, so mind my ignorance. I am a little confused what is happening here where the token is set to provider just moments after being set to provider.provide

Copy link
Member Author

@BrunnerLivio BrunnerLivio May 7, 2019

Choose a reason for hiding this comment

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

You’re right that is not needed. I had to do a lot of tinkering back then and seems like a unneeded statement is still present .

Nonetheless, this PR is currently on ice because I do not find time to look further into this at the moment. The current PR behaves similarly as in Angular. The problem is; Angular Injectables are global, whereas in Nest they are module specific (only public by exporting it). This makes it quite hard to implement in Nest but we hopefully get there!

@BrunnerLivio
Copy link
Member Author

Closed in favor of #2460. I think I got it working, but it slightly different (Multi Providers will be handled as globals). I gotta need to take a break, it is 35 degrees here, my brain is boiling and I have no idea why my code even works (?!?). Will follow up tomorrow :-)

@lock
Copy link

lock bot commented Sep 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants