feat(effects-ng): Directive effects

[EricPoul]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngneat/effects/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have 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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[EricPoul]

That's the implementation. What do you think about it @NetanelBasal? It seems to be a little strange, and unusual, maybe because of the new API.

[NetanelBasal]

Yes, that's exactly what I meant.

[EricPoul]

Faced with new and new corner cases. But this implementation should cover everything.

[NetanelBasal]

@EricPoul, I'm trying to understand the use cases you're trying to handle.

Let's say I have an effect named TodosEffects. It doesn't matter where I register it. I want to have one subscription.

Cases:

root - TodosEffects - subscribe here

---

root - TodosEffects - subscribe here
env injector - TodosEffects
useDirectiveEffects- TodosEffects

---

env injector - TodosEffects - subscribe here
useDirectiveEffects- TodosEffects

---

root - TodosEffects - subscribe here
useDirectiveEffects- TodosEffects

---

useDirectiveEffects- TodosEffects - subscribe here

What am I missing?

[EricPoul]

TodoComponent(instance 1) - useDirectiveEffects - TodosEffects(instance 1) - subscribe here
TodoComponent(instance 2) - useDirectiveEffects - TodosEffects(instance 2) - skip subscribing

TodoComponent(instance 1).onDestory() - effects of TodosEffects(instance 1) should still be alive
TodoComponent(instance 2).onDestory() - we need to unsubscribe from the effects of TodosEffects(instance 1) but TodosEffects(instance 2) doesn't have it so we need to the effects of TodosEffects(instance 1).

Although TodosEffects(instance 1) and TodosEffects(instance 2) has the same names of effects, effects themself are different and have different references.

[NetanelBasal]

but why do we care about references? The suggestion was to unique it based on the constructor name + the effect prop name. TodosEffect/getTodos

[EricPoul]

EffectsManager.effects is a WeakMap so if I want to unsubscribe from the effect I need to pass the exact ref of the effect to the EffectsManager.removeEffects(...).

[NetanelBasal]

And that will stay. We will have a global Set that registers the unique identifiers. Before registering an effect, you will check if it exists in the Set. if that's the case, skip it.

[EricPoul]

It's what I do.

TodoComponent(instance 1) - useDirectiveEffects - TodosEffects(instance 1) - subscribe here
TodoComponent(instance 2) - useDirectiveEffects - TodosEffects(instance 2) - I skip subscription here
TodoComponent(instance 3) - useDirectiveEffects - TodosEffects(instance 3) - I skip subscription here

TodoComponent(instance 1) was destroyed - effects of TodosEffects(instance 1) should still be alive because TodoComponent(instance 2) and TodoComponent(instance 3) are still alive.

TodoComponent(instance 3) was destroyed - effects of TodosEffects(instance 1) should still be alive because TodoComponent(instance 2) is still alive.

TodoComponent(instance 2) was destroyed - effects of TodosEffects(instance 1) should be removed.
Only one way to unsubscribe from the effects of TodosEffects(instance 1) is to have them stored somewhere(what I do now)(because TodosEffects(instance 1) has already been destroyed).
Also, I have to know that the last instance of TodosEffects was destroyed, and for that, I store sources amount.

[EricPoul]

The problem that I described is only with components and their chaotic order of being destroyed. For provideEffect a plain Set with a unique id will be enough but for components effects, it's not.

[NetanelBasal]

I see, but that's not the purpose of the feature as I see it. It should be only for development and should immediately throw when seeing that you try to use the same effect multiple times. We don't need to manage subscriptions.

I think it's a bad design if someone gets to the case you described.

[NetanelBasal]

We help by saying "Hi, you're registering the same effect multiple times. That's not a good thing to do"

[NetanelBasal]

But you're the consumer of the library. If you think we should leave your implementation, let it be.

[NetanelBasal]

Let's leave your code. You already wrote it. I'd just add a test for the exact scenario you describe here

[EricPoul]

I really don't know how is better😄
It'll be confusing if provideEffects do not subscribe twice and useDirectiveEffects does.

[NetanelBasal]

useDirectiveEffects should be the same as provideEffects.

[NetanelBasal]

Great work. We just need to update the docs.

[EricPoul]

I'll go through the changes once more and update the docs after. Once I do I'll mark this pr as ready.