Effects: Issue with effects instantiating multiple times creates multiple same handlers

[simply10w]

Minimal reproduction of the bug/regression with instructions:

https://stackblitz.com/edit/ngrx-seed-ljumtv

Expected behavior:

Not sure what is considered expected behavior in this case, but I think this is a breaking change. The issue is caused in 7.0.0-beta.0 after the addition of a new feature that allows instantiating the same effect class multiple times.

It should be change from this PR: #1249

The issue occurs when you have a shared module that will group some store logic, specifically effects.

In versions before 7.0.0-beta.0, if you import that shared store to 2 different lazy loaded modules, only one effect is instantiated thus when action is dispatched we only handle that action once.

In 7.0.0-beta.0 after the addition of that new feature that allows instantiating the same effect it will instantiate both of those effects and thus we will have 2 same handlers.

We are just now upgrading our enterprise application, its using a monorepo approach and has a lot of those shared stores as libraries for manipulating different collections that are shared across features.

On top of that, we utilize lazy loading of each feature, and those are also separated into libraries. Since most of those libraries are plug and play, they hide their implementation details, so when feature depends on shared store logic it just imports it, it does not expect that it is already provided.

That way:

1. they can be bundled together if nothing else depends on the same logic.
2. App module does not need to know on what does that feature depend.

Before the upgrade, this worked without a problem because effects were created once, similarly how reducer is being created if using the same principle.

After the upgrade if you start going through features it will instantiate the same effect multiple times, so for example, if you want to update/delete an entity, it will trigger the call for each handler that was created.

A possible solution is instead of importing those shared store modules into features, we could just add them to the app module, so they are created once, but that way it creates a leaky abstraction, since app module needs to know on which store part certain feature depends. Also it will be bundled together with the main bundle and those might not be necessarily needed at that point in time.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

ngrx/effects: 7.0.0-beta.0
Not affected by angular, node or browser

Other information:

I would be willing to submit a PR to fix this issue

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[timdeschryver]

You make a valid point here, forgot about this use case when accepting the referenced PR.

If we want to support both cases, I think we should give the option to the user to add an identifier while manually adding effects.

The first thing that comes to mind is doing something like the following:

 addEffects(
    effectSourceInstance: any,
    identifier = getSourceForInstance(effectSourceInstance)
  ) {
    this.next({ effectSourceInstance, identifier });
  }

This would have the same behavior as previous versions of NgRx, but gives the opportunity to register multiple effects of the same class with:

constructor(effectSources: EffectSources) {
    effectSources.addEffects(new ConfigurableEffect("configA"), identifier: 'configA');
    effectSources.addEffects(new ConfigurableEffect("configB"), identifier: 'configB');
  }

Thoughts?

cc: @dummdidumm

[dummdidumm]

I don't think this is a bug on ngrx' side. Effects now behave similar to normal Angular Services. If you provide the same service in different modules, they are instanciated multiple times, too. That's why the forRoot-pattern exists. But I agree that the change in behavior is a breaking change, I also did not think of this situation.

Maybe the forFeature-function goes back to the old behavior and addEffects gets an optional identifier param, which

• when set, will be taken into account to prevent duplicates (EffectsModule.forRoot / EffectsModule.forFeature will use that)
• when not set, will work the same as it now does (effect will be instanciated as many times as I call addEffects)

[manklu]

@dummdidumm

I don't think this is a bug on ngrx' side. Effects now behave similar to normal Angular Services.

I don't think so. In Angular each service method is only called once and not in each instance.
Big difference.

[dummdidumm]

You are right, each service method is only called once, but you may end up with multiple instances of the same service: https://angular.io/guide/singleton-services#forroot "If a module provides both providers and declarations (components, directives, pipes) then loading it in a child injector such as a route, would duplicate the provider instances. The duplication of providers would cause issues as they would shadow the root instances, which are probably meant to be singletons."

[manklu]

@dummdidumm

The duplication of providers would cause issues as they would shadow the root instances, which are probably meant to be singletons."

AFAIK Only if they have a state that should be shared by all clients. Otherwise, it's just a waste of memory but anything work as expected.