feat(effects): add user provided effects to EffectsModule.forFeature

[leon-marzahn]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

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
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

It is currently not possible to register effects with an InjectionToken like it's possible in StoreModule.forFeature
Closes #2232

What is the new behavior?

It's now possible to pass an InjectionToken to either USER_PROVIDED_EFFECTS to register Effects. E.g.

@NgModule({
  imports: [
    EffectsModule.forFeature(),
  ],
  providers: [
    SomeEffect,
    SomeOtherEffect,
    {
      provide: USER_PROVIDED_EFFECTS,
      multi: true,
      useValue: [SomeEffect, SomeOtherEffect],
    },
  ],
})
class FeatureModuleUsingStore {
}

This is useful to create consistency with StoreModule.forFeature and helps in making it possible to reducer boilerplate for NGRX. You could load effects by providers, which creates possibilities, e.g. putting actions, reducers and effects into a class, that is loaded via dependency injection.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

This is my first attempt at contributing to any project, so please be easy on me :D
I couldn't write documentation yet, as i don't really know if this needs any documentation. I would appreciate some feedback and am willing for change in this.

[ngrxbot]

Preview docs changes for dca891c at https://previews.ngrx.io/pr2231-dca891c/

[leon-marzahn]

This also fixes #1432 (Although it's closed, i don't know if he found a solution)

[leon-marzahn]

Now that i think about it, it would be even better if you can pass instances of effects as well. So the token would be of type InjectionToken<(Type<any> | any)[]>. And it would only inject them if they are of type Type. Otherwise it can just use the instance.

[timdeschryver]

I'm not sure which use cases this feature has, in the issue it mentions that it reduces boilerplate but I'm not seeing how this can reduce it.

But it got me thinking about a different API, instead of providing the token in the forFeature function, what about having a similar API to USER_PROVIDED_META_REDUCERS? This would look as follows:

providers: [
  {
    provide: USER_PROVIDED_EFFECTS,
    useValue: [FooEffect, BarEffect]
  }
]

The above only if it makes sense to have a similar feature...

If we implement this we should also add the feature to use a token in the EffectsModule.forRoot() function for consistency.

[leon-marzahn]

@timdeschryver Because you said you don't know how this can reduce boilerplate. Essentially you would be able to create effects, reducers and etc. at runtime, which allows you to do that in any way, shape or form. In my example i would use a base class BaseStore and some module that creates effects/reducers from this class. The class would have functions like createReducer and createEffects.

[leon-marzahn]

Something like this: https://stackblitz.com/edit/angular-rm3eiu
Obviously this doesn't work and is very quick and dirty. But this should let you get the idea

[brandonroberts]

What's the status on this?

[leon-marzahn]

I couldn't work on this over the holidays, i would continue today with the changes suggested

[leon-marzahn]

@brandonroberts & @timdeschryver I've made changes to this and implemented it as suggested by @timdeschryver. Also, i don't know how to fix the conflict, as every time i fix it, the githooks break it again :D

[brandonroberts]

@PsychoPflanze are you rebased on latest master?

[leon-marzahn]

@PsychoPflanze are you rebased on latest master?

@brandonroberts I am yes, but the githooks correct the formatting of a specific import, which causes a conflict

[timdeschryver]

@PsychoPflanze can you try to rebase again?
You should pick the export with the most imports:

-export { EffectsFeatureModule } from './effects_feature_module';
+export {
+  ROOT_EFFECTS_INIT,
+  rootEffectsInit,
+  EffectsRootModule,
+} from './effects_root_module';

[leon-marzahn]

@timdeschryver & @alex-okrushko Any updates?

[leon-marzahn]

I've figured out how to use forks, and the pull request is now up to date with master

[timdeschryver]

I will have a look at the PR later this week.

[leon-marzahn]

Alright, i've fixed the tests and merged the tokens to USER_PROVIDED_EFFECTS. Also i managed to test the order better now, thanks for the change, it made it easier to test :D

[leon-marzahn]

There is a test failing i haven't touched, what should i do about it?

[leon-marzahn]

@timdeschryver or @brandonroberts or @alex-okrushko

[timdeschryver]

I think the test fails because of the changes made in the PR.
I'll have a look at it later today.

[timdeschryver]

The test fails because in the module, multiple forFeatures are called and they all override eachother.
As a fix, make the token a multi token (and reflect this change in the createEffects method):

{
          provide: _FEATURE_EFFECTS,
          useValue: featureEffects,
          multi: true,
},

@leon-marzahn

[leon-marzahn]

The test fails because in the module, multiple forFeatures are called and they all override eachother.
As a fix, make the token a multi token (and reflect this change in the createEffects method):

{
          provide: _FEATURE_EFFECTS,
          useValue: featureEffects,
          multi: true,
},

@leon-marzahn

Alright, thank you, i didn't notice 😄

[leon-marzahn]

I just remembered i will have to adjust the documentation

[timdeschryver]

LGTM - just one more comment.
When you've updated the docs, feel free to ping me and we then it's ready to be merged for me :)
Nvm, the docs are already updated 🎉

[timdeschryver]

👍

[brandonroberts]

Thanks @leon-marzahn!

[vash72]

When there will be a chance to make a release? or may you point me out how to build the package myself from master branch

[brandonroberts]

Builds from the master branch are here https://ngrx.io/guide/nightlies#effects