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

Remove deprecated .forRoot() from the code #3015

Closed
maxokorokov opened this issue Feb 12, 2019 · 3 comments
Closed

Remove deprecated .forRoot() from the code #3015

maxokorokov opened this issue Feb 12, 2019 · 3 comments

Comments

@maxokorokov
Copy link
Member

Should have been removed in 4.0.0, but was forgotten.
Marked as deprecated since 3.0.0.

@maxokorokov maxokorokov added this to the 5.0 milestone Feb 12, 2019
@maxokorokov maxokorokov mentioned this issue Feb 26, 2019
3 tasks
@benouat
Copy link
Member

benouat commented Apr 4, 2019

I am not really sure we should do it.

When animations will land #2817 we will have to introduce an API to globally configure ng-bootstrap.

A natural solution (at least to me) would be to use forRoot for that (or any other static method...)

@NgModule({
  imports: [
    NgbModule.forRoot({ enableAnimations: true })
  ]
})
export class AppModule {}

This solution is somehow used in plenty of other libraries:

  • Angular Router with RouterModule.forRoot(routes, { options }) or RouterModule.forChild(routes)
  • Ngrx with StoreModule.forRoot({ options }), EffectModule.forRoot([ effects here ]) or EffectModule.forFeature([ effects here ])

Also, knowing we deliver per-widget modules, we should also keep the same mechanism to allow people using only one or two widgets modules to also configure them.

@NgModule({
  imports: [
    NgbTypeaheadModule.forRoot({ enableAnimations: true }),
    NgbModalModule.forRoot({ enableAnimations: true })
  ]
})
export class AppModule {}

@maxokorokov @pkozlowski-opensource WDYT ? Maybe I am completely off topic here, and using raw DI would be enough... I don't know.

By raw DI I mean something like that

@NgModule({
  imports: [
    NgbTypeaheadModule,
    NgbModalModule
  ],
  providers: [
    /* Using a Token ? */
    { provide: NGB_CONFIG, useValue: { enableAnimations: true } },
    /* Using a Config Class ? */
    { provide: NgbConfig , useFactory: myNgbConfigFactory },
  ]
})
export class AppModule {}

Myself I prefer the .forRoot() over the DI.

@benouat benouat modified the milestones: 5.0, 5.0.0-rc.0 Jun 13, 2019
benouat pushed a commit that referenced this issue Jun 24, 2019
Closes #3015 

BREAKING CHANGE: Importing any ng-bootstrap module via `.forRoot()` has now been completely removed.
The only supported way is the one documented in the [getting started](https://ng-bootstrap.github.io/#/getting-started#installation) page.

```typescript
import {NgbModule} from '@ng-bootstrap/ng-bootstrap';

@NgModule({
  ...
  imports: [NgbModule, ...],
  ...
})
export class YourAppModule {
}
```
mijohnsmith pushed a commit to mijohnsmith/ng-bootstrap that referenced this issue Jun 27, 2019
Closes ng-bootstrap#3015 

BREAKING CHANGE: Importing any ng-bootstrap module via `.forRoot()` has now been completely removed.
The only supported way is the one documented in the [getting started](https://ng-bootstrap.github.io/#/getting-started#installation) page.

```typescript
import {NgbModule} from '@ng-bootstrap/ng-bootstrap';

@NgModule({
  ...
  imports: [NgbModule, ...],
  ...
})
export class YourAppModule {
}
```
@zamzowd
Copy link

zamzowd commented Jul 18, 2019

@benouat Did you guys come up with a planned approach for how to take configuration values when registering the module, per your concerns written above?

@benouat
Copy link
Member

benouat commented Aug 12, 2019

@zamzowd nope 😞 not yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants