feat(effects): make resubscription handler overridable

[zakhenry]

PR Checklist

Please check if your PR fulfils 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?

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

Behavior of resubscription to closed observables is not configurable

Closes #2294

What is the new behavior?

Behavior of resubscription to closed observables is now configurable by injecting a substitute implementation.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Impact is relatively low - it is a simple rename of a field that was introduced in v8. See @alex-okrushko's comment - he found nowhere in google that is using this feature yet.

Migration path is a simple rename, an automated migration with a schematic is theoretically possible (I've not done this before but willing to learn).

[ngrxbot]

Preview docs changes for f2e40f2 at https://previews.ngrx.io/pr2295-f2e40f2/

[brandonroberts]

Closing this along with #2294

[zakhenry]

@brandonroberts would you mind reopening this PR please given #2294 was reopened?

[alex-okrushko]

Correct me if I'm wrong, if we set EFFECTS_ERROR_HANDLER in any of the modules, it would override the default effects error handle for all the effects classes.

I think we need something that we can pass to a particular EffectsModule either forRoot or forChild and it would apply only to that group of Effects. WDYT?

[zakhenry]

Correct me if I'm wrong, if we set EFFECTS_ERROR_HANDLER in any of the modules, it would override the default effects error handle for all the effects classes.

I think we need something that we can pass to a particular EffectsModule either forRoot or forChild and it would apply only to that group of Effects. WDYT?

@alex-okrushko Hmmm this is an interesting thought - I'm not sure that I'm convinced that the effects grouping is the level that developers would want to customise the behaviour, because at that point they would have limited ability to customise the behavior on a per-class basis.

Perhaps the better solution to address that case would be to (maybe later) introduce a new hook, and if developers desire grouping within a module it could be done through inheritance or a custom decorator if they so choose.

e.g.

class MyEffects implements EffectErrorHandler {

   ngrxEffectErrorHandler(action$: Observable<Action>, errorHandler: ErrorHandler) {
     // ...
   }

   effect$ = createEffect(...);

}

This would be more expected IMO considering similar control behavior with OnRunEffects being class hook based.

[alex-okrushko]

This is still on my radar - will try to get some time to review it tomorrow.

[zakhenry]

@alex-okrushko yea no problem, whenever is fine

[alex-okrushko]

Thanks for working on it 👍

Migration path is a simple rename, an automated migration with a schematic is theoretically possible (I've not done this before but willing to learn).

Let's look into that as well - in another PR :)

[timdeschryver]

I like the API this PR provides, good job 👍

Migration path is a simple rename, an automated migration with a schematic is theoretically possible (I've not done this before but willing to learn).

Yes, this is possible and should be a simpler one.
If you want I can grab this one after this PR lands, or assist you in making a PR - the choice is yours 🙂

[zakhenry]

@timdeschryver thanks, I'm going to take a crack at the migration PR, I'll let you know if/when I need a hand. I've got the basics working already, just need to narrow the visitor so it's not just doing a dangerous global find/replace. I'll open the PR this weekend to discuss over. I'll keep this PR for the actual change 👍