Closed EricPoul closed 1 year ago
Run & review this pull request in StackBlitz Codeflow.
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.
Yes, that's exactly what I meant.
Faced with new and new corner cases. But this implementation should cover everything.
@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.
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?
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.
but why do we care about references? The suggestion was to unique it based on the constructor name + the effect prop name. TodosEffect/getTodos
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(...)
.
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.
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.
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.
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.
We help by saying "Hi, you're registering the same effect multiple times. That's not a good thing to do"
But you're the consumer of the library. If you think we should leave your implementation, let it be.
Let's leave your code. You already wrote it. I'd just add a test for the exact scenario you describe here
I really don't know how is better😄
It'll be confusing if provideEffects
do not subscribe twice and useDirectiveEffects
does.
useDirectiveEffects should be the same as provideEffects.
I'll go through the changes once more and update the docs after. Once I do I'll mark this pr as ready
.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information