feat(effects): add OnIdentifyEffects interface to register multiple effect instances #1448

timdeschryver · 2018-11-27T18:35:26Z

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?

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

Closes #1443

What is the new behavior?

This change reverts #1249 in order to only instantiate effects once (e.g. when imported in different features).
This PR adds a way to identify effects while using the addEffects method in order to resolve #1246.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

brandonroberts · 2018-11-29T13:26:59Z

What do you think about making the identifier something that is pulled from the effect? Like a lifecycle method that is called from the effect instance in the groupBy if present? I'd like something more defined for the identifier.

timdeschryver · 2018-11-29T14:00:10Z

I think that should also work, something like this right?

export class FooEffects implements EffectIdentifier {
  identifier = 'Foo';
}

brandonroberts · 2018-11-29T14:21:20Z

Yep

coveralls · 2018-11-29T16:05:30Z

Coverage remained the same at 88.342% when pulling 3a6d41f on pr/effect-sources-identifier into 6a754aa on master.

dummdidumm · 2018-11-29T18:09:32Z

I think different effect classes should be able to get instantiated even if they have the same identifier.
So if class EffectsA has identifier "abc" and EffectsB also has identifier "abc", they should both get instantiated. With the current implementation this is not the case I think.

dummdidumm · 2018-11-29T18:14:56Z

I also think that getting the identifier from the class instance is not a good idea. People now have to know that "identifier" is a reserved property that the effects library uses. If someone does not implement the interface but still uses a variable called identifier for something else, strange things could happen.

timdeschryver · 2018-11-29T18:30:29Z

@dummdidumm I agree with:

I think different effect classes should be able to get instantiated even if they have the same identifier.
So if class EffectsA has identifier "abc" and EffectsB also has identifier "abc", they should both get instantiated. With the current implementation this is not the case I think.

I think identifier is perhaps a bit too general, ngrxIdentifier or effectIdentifier might be better.

timdeschryver · 2018-11-30T06:58:37Z

@dummdidumm Why should it be a,a,b,b?

dummdidumm · 2018-11-30T07:02:06Z

SourceWithIdentifier is instantiated with identifier, "a" two times, SourceWithIdentifier 2 is instantiated with identifier "b" two times . It should be "a","b" for each effect class. Then all four should get instantiated.

timdeschryver · 2018-11-30T09:26:51Z

You're right, it behaves that way but the test didn't match the description.

brandonroberts · 2018-11-30T13:30:22Z

modules/effects/src/lifecycle_hooks.ts

@@ -0,0 +1,6 @@
+export interface EffectIdentifier {


Need some more doc here for the API page 😉

/** * @description * Interface to set an identifier for effect instances. * * By default, each Effects class will only be registered * once regardless of how many times the Effect class * is loaded. By implementing this interface, you define * a unique identifier to register an Effects class instance * multiple times. * * @usageNotes * * ### Set an identifier for an Effects class * * ``` * Example code here * ``` */ export interface EffectIdentifier { /** * @description * String identifier to differentiate effect instances. */ effectIdentifier: string; }

dummdidumm · 2018-11-30T15:34:39Z

I still think that setting the identifier on the class is not the best possible solution.

people could use a property of the same name in a different context without knowing and get strange results
angular's dependency injection needs to know where the constructor property comes from. So you cannot provide that effect class to the effectsmodule through forFeature/forRoot without workarounds
in my opinion conceptually it is not the responsibility of the class itself to know "who it is". The addEffects function should know that instead.

What are your arguments against going with the initial proposal ?

timdeschryver · 2018-12-01T15:44:01Z

I don't have a favorite way of solving this (the first proposal vs the interface).

You're right with your second point @dummdidumm , but I'm not quite agreeing with your first point. Angular does more or less do the same with their lifecycle hooks, plus it opens up opportunities to introduce more of these in NgRx e.g. in #1447.

dummdidumm · 2018-12-01T16:27:52Z

You are right, it opens up more possibilities, and yeah, angular does kind of the same, so I'm fine with it I guess. Regarding the API: Maybe we should decide on a prefix (like angular did with their lifecycle hooks with "ng"). "ngrx" would seem like a natural fit.
There is also "ngrxOnRunEffects" already which would be one more argument towards going with the interface approach.

timdeschryver · 2018-12-01T19:16:40Z

I forgot about ngrxOnRunEffects for a second 😅 .

To stay consistent I would want to rename the property effectIdentifier to ngrxEffectIdentifier and also move the OnRunEffects interface to the lifecycle_hooks, would that be OK @brandonroberts ?

brandonroberts · 2018-12-03T14:16:50Z

@timdeschryver that's fine with me because it doesn't change the public API. I'd rather see this implemented as a method if we're being similar to ngrxOnRunEffects and have it be ngrxOnIdentify which is a little more descriptive.

timdeschryver · 2018-12-03T18:07:09Z

👍 I'll implement it the way you suggested.
Thanks for the feedback @dummdidumm and @brandonroberts !

timdeschryver · 2018-12-03T18:11:08Z

What about ngrxOnIdentifyEffects (sorry, I know that I'm annoying 😅 )?

brandonroberts · 2018-12-03T18:13:14Z

😂

That's fine. You're identifying this set of effects 👍

brandonroberts · 2018-12-03T20:18:40Z

modules/effects/src/effect_sources.ts

@@ -47,3 +55,34 @@ export class EffectSources extends Subject<any> {
    );
  }
 }
+
+function effectsInstance(source: any) {


Suggested change

function effectsInstance(source: any) {

function effectsInstance(sourceInstance: any) {

brandonroberts · 2018-12-03T20:19:43Z

modules/effects/src/lifecycle_hooks.ts

+ * @description
+ * Interface to set an identifier for effect instances.
+ *
+ * By default, each Effects class will only be registered


Suggested change

* By default, each Effects class will only be registered

* By default, each Effects class is registered

brandonroberts · 2018-12-03T20:21:25Z

projects/ngrx.io/content/guide/migration/v7.md

@@ -84,6 +84,12 @@ export class MyEffects {
 }
 ```

+### reserved property `ngrxOnIdentifyEffects`


I don't think this needs to be in the migration doc as it doesn't break existing behavior. We should update the PR to a feat that adds the ngrxOnIdentifyEffects interface.

The reason why I added it to the migration is that if someone would already use this property. But the chance that this is the case is probably zero.

brandonroberts · 2018-12-03T20:23:56Z

modules/effects/src/index.ts

 export { EffectNotification } from './effect_notification';
 export { ROOT_EFFECTS_INIT } from './effects_root_module';
 export { UPDATE_EFFECTS, UpdateEffects } from './effects_feature_module';
+export {
+  OnIdentifyEffects,


It should be sufficient to only export the interfaces publicly

brandonroberts · 2018-12-03T21:15:14Z

@timdeschryver one more minor nit. Will you fix the first commit message with OnIdentifyEffects. It currently says OnIdentityEffects. I'm going to land the commits as-is.

timdeschryver added the 7.x label Nov 27, 2018

timdeschryver force-pushed the pr/effect-sources-identifier branch from 684117e to 4c96ffc Compare November 29, 2018 16:05

timdeschryver force-pushed the pr/effect-sources-identifier branch from 4c96ffc to 099f5e3 Compare November 29, 2018 16:06

brandonroberts requested changes Nov 30, 2018

View reviewed changes

timdeschryver force-pushed the pr/effect-sources-identifier branch 3 times, most recently from 1f1170b to c0feb45 Compare December 3, 2018 19:45

brandonroberts requested changes Dec 3, 2018

View reviewed changes

brandonroberts changed the title ~~fix(Effects): instantiate effect once when used in multiple features~~ feat(effects): add OnIdentifyEffects interface to register multiple effect instances Dec 3, 2018

timdeschryver force-pushed the pr/effect-sources-identifier branch from c0feb45 to 3a6d41f Compare December 3, 2018 20:42

brandonroberts approved these changes Dec 3, 2018

View reviewed changes

timdeschryver added 2 commits December 4, 2018 07:44

feat(Effects): add OnIdentifyEffects interface

efa3cb9

refactor(Effects): move OnRunEffects to lifecycle_hooks

8980112

timdeschryver force-pushed the pr/effect-sources-identifier branch from 3a6d41f to 8980112 Compare December 4, 2018 06:47

brandonroberts merged commit b553ce7 into master Dec 4, 2018

brandonroberts deleted the pr/effect-sources-identifier branch December 4, 2018 13:19

timdeschryver restored the pr/effect-sources-identifier branch March 27, 2019 21:35

timdeschryver deleted the pr/effect-sources-identifier branch March 27, 2019 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(effects): add OnIdentifyEffects interface to register multiple effect instances #1448

feat(effects): add OnIdentifyEffects interface to register multiple effect instances #1448

timdeschryver commented Nov 27, 2018

brandonroberts commented Nov 29, 2018

timdeschryver commented Nov 29, 2018

brandonroberts commented Nov 29, 2018

coveralls commented Nov 29, 2018 •

edited

Loading

dummdidumm commented Nov 29, 2018

dummdidumm commented Nov 29, 2018

timdeschryver commented Nov 29, 2018

timdeschryver commented Nov 30, 2018

dummdidumm commented Nov 30, 2018 •

edited

Loading

timdeschryver commented Nov 30, 2018

brandonroberts Nov 30, 2018

dummdidumm commented Nov 30, 2018 •

edited

Loading

timdeschryver commented Dec 1, 2018

dummdidumm commented Dec 1, 2018 •

edited

Loading

timdeschryver commented Dec 1, 2018

brandonroberts commented Dec 3, 2018

timdeschryver commented Dec 3, 2018

timdeschryver commented Dec 3, 2018

brandonroberts commented Dec 3, 2018 •

edited

Loading

brandonroberts Dec 3, 2018

brandonroberts Dec 3, 2018

brandonroberts Dec 3, 2018

timdeschryver Dec 3, 2018

brandonroberts Dec 3, 2018

brandonroberts commented Dec 3, 2018

	function effectsInstance(source: any) {
	function effectsInstance(sourceInstance: any) {

	* By default, each Effects class will only be registered
	* By default, each Effects class is registered

feat(effects): add OnIdentifyEffects interface to register multiple effect instances #1448

feat(effects): add OnIdentifyEffects interface to register multiple effect instances #1448

Conversation

timdeschryver commented Nov 27, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

brandonroberts commented Nov 29, 2018

timdeschryver commented Nov 29, 2018

brandonroberts commented Nov 29, 2018

coveralls commented Nov 29, 2018 • edited Loading

dummdidumm commented Nov 29, 2018

dummdidumm commented Nov 29, 2018

timdeschryver commented Nov 29, 2018

timdeschryver commented Nov 30, 2018

dummdidumm commented Nov 30, 2018 • edited Loading

timdeschryver commented Nov 30, 2018

brandonroberts Nov 30, 2018

Choose a reason for hiding this comment

dummdidumm commented Nov 30, 2018 • edited Loading

timdeschryver commented Dec 1, 2018

dummdidumm commented Dec 1, 2018 • edited Loading

timdeschryver commented Dec 1, 2018

brandonroberts commented Dec 3, 2018

timdeschryver commented Dec 3, 2018

timdeschryver commented Dec 3, 2018

brandonroberts commented Dec 3, 2018 • edited Loading

brandonroberts Dec 3, 2018

Choose a reason for hiding this comment

brandonroberts Dec 3, 2018

Choose a reason for hiding this comment

brandonroberts Dec 3, 2018

Choose a reason for hiding this comment

timdeschryver Dec 3, 2018

Choose a reason for hiding this comment

brandonroberts Dec 3, 2018

Choose a reason for hiding this comment

brandonroberts commented Dec 3, 2018

coveralls commented Nov 29, 2018 •

edited

Loading

dummdidumm commented Nov 30, 2018 •

edited

Loading

dummdidumm commented Nov 30, 2018 •

edited

Loading

dummdidumm commented Dec 1, 2018 •

edited

Loading

brandonroberts commented Dec 3, 2018 •

edited

Loading