fix(effects-directive): unregister all effects of a provider

ngneat / effects

🪄 A framework-agnostic RxJS effects implementation

https://www.netbasal.com

MIT License

63 stars 11 forks source link

fix(effects-directive): unregister all effects of a provider #73

Closed ntziolis closed 2 months ago

ntziolis commented 2 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

[x] 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? Ensures correct behavior of directive when on destroy is called

[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?

Only the last (order) effect in a provider was unregistered.

Issue Number: #72

What is the new behavior?

All effects are of a provider are unregistered.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

stackblitz[bot] commented 2 months ago

Run & review this pull request in StackBlitz Codeflow.

NetanelBasal commented 2 months ago

@EricPoul can you please review this?

ntziolis commented 2 months ago

Please let me know if you feel I should add additional unit tests. I added just enough to make them fail (multiple effects in a single provider). And the added a fix and retested.

EricPoul commented 2 months ago

LGTM

No need for an extra test. Just check that spy3 is called the same number of times as spy1 in the existing tests.

ntziolis commented 2 months ago

No need for an extra test. Just check that spy3 is called the same number of times as spy1 in the existing tests.

Done. That said I used spy3 to cover some additional edge cases instead of just mirroring spy1.

EricPoul commented 2 months ago

Thank you for the fix!

ntziolis commented 2 months ago

One note. While I would not consider this a breaking change as such, since it just ensures the expected behavior. It does change significantly how the directive functions and might affect existing apps in a way that might not be easy to see for other devs. Not sure what the right way would be to go about this.

EricPoul commented 2 months ago

It should be fine generally. If someone faces a problem, it's because their code has worked wrong. While it is breaking change in some way, I don't see a problem.

EricPoul commented 2 months ago

@NetanelBasal I created a tag and updated version but it still isn't available for downloading/ Could you check what I missed?

NetanelBasal commented 2 months ago

I need to publish it

NetanelBasal commented 2 months ago

Done