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(effects): add user provided effects to EffectsModule.forFeature #2231

Merged
merged 13 commits into from
Mar 17, 2020

Conversation

leon-marzahn
Copy link
Contributor

@leon-marzahn leon-marzahn commented Nov 8, 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
[ ] 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
Copy link
Collaborator

ngrxbot commented Nov 8, 2019

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

@leon-marzahn leon-marzahn marked this pull request as ready for review November 8, 2019 10:41
@leon-marzahn
Copy link
Contributor Author

leon-marzahn commented Nov 8, 2019

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

@leon-marzahn
Copy link
Contributor Author

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.

@leon-marzahn leon-marzahn force-pushed the feat/effects-injection-token branch from 82a3a57 to ebf724f Compare November 8, 2019 11:05
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

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.

modules/effects/src/effects_module.ts Outdated Show resolved Hide resolved
@leon-marzahn
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

What's the status on this?

@leon-marzahn
Copy link
Contributor Author

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

@leon-marzahn leon-marzahn force-pushed the feat/effects-injection-token branch 3 times, most recently from acf934a to cbccef2 Compare January 16, 2020 10:00
@leon-marzahn leon-marzahn changed the title feat(effects): add injection token to EffectsModule.forFeature feat(effects): add user provided effects to EffectsModule.forFeature Jan 16, 2020
@leon-marzahn
Copy link
Contributor Author

@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
Copy link
Member

@PsychoPflanze are you rebased on latest master?

@leon-marzahn
Copy link
Contributor Author

@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
Copy link
Member

timdeschryver commented Jan 16, 2020

@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 leon-marzahn force-pushed the feat/effects-injection-token branch from f79529a to ac9231e Compare January 17, 2020 16:40
@leon-marzahn
Copy link
Contributor Author

@timdeschryver & @alex-okrushko Any updates?

@leon-marzahn leon-marzahn force-pushed the feat/effects-injection-token branch 2 times, most recently from ac9231e to b6ea465 Compare January 22, 2020 12:29
@leon-marzahn
Copy link
Contributor Author

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

@timdeschryver
Copy link
Member

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

modules/effects/src/effects_module.ts Outdated Show resolved Hide resolved
modules/effects/src/effects_module.ts Outdated Show resolved Hide resolved
modules/effects/src/effects_module.ts Outdated Show resolved Hide resolved
@leon-marzahn
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@leon-marzahn
Copy link
Contributor Author

@timdeschryver
Copy link
Member

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

@timdeschryver
Copy link
Member

timdeschryver commented Mar 9, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

I just remembered i will have to adjust the documentation

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

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 🎉

modules/effects/spec/integration.spec.ts Show resolved Hide resolved
modules/effects/spec/integration.spec.ts Show resolved Hide resolved
modules/effects/src/effects_module.ts Outdated Show resolved Hide resolved
Leon Marzahn and others added 3 commits March 12, 2020 10:43
Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

👍

@brandonroberts brandonroberts removed the Need Discussion Request For Comment needs input label Mar 17, 2020
@brandonroberts brandonroberts merged commit 59ce3e2 into ngrx:master Mar 17, 2020
@brandonroberts
Copy link
Member

Thanks @leon-marzahn!

@vash72
Copy link

vash72 commented Mar 18, 2020

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
Copy link
Member

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

@leon-marzahn leon-marzahn deleted the feat/effects-injection-token branch March 18, 2020 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Accept InjectionToken in EffectsModule.forFeature
7 participants