fix(router): complete router events on dispose

[arturovt]

In this commit, we manually complete the events subject to remove all active observers and enable granular garbage collection, as users may forget to unsubscribe manually when subscribing to router.events.

[crisbeto]

It looks like this is breaking internally so it'll likely break users externally too. The problem is that if someone is subscribing using the first operator and the observable completes before it has emitted a value, rxjs will throw an error.

[arturovt]

@crisbeto what do you think if we call unsubscribe() on the subject in order to clean up the list of observers (w/o calling complete() on observers)? This is only to prevent any breaking changes. I could leave a comment that calling complete() is unsafe.

[crisbeto]

We can try it out to see if it breaks anything internally.

[arturovt]

Changed to unsubscribe().

[crisbeto]

This time there were only two test failures on a global run, although I'm not quite sure why it's failing. Here's the stack trace:

ObjectUnsubscribedError: object unsubscribed

error properties: null({ constructor: Function })
Error
at _super (rxjs/src/internal/util/createErrorClass.ts:33:22)
      at new ObjectUnsubscribedErrorImpl (rxjs/src/internal/util/ObjectUnsubscribedError.ts:40:7)
      at Subject._throwIfClosed (rxjs/src/internal/Subject.ts:69:13)
      at Subject._trySubscribe (rxjs/src/internal/Subject.ts:134:10)
      at Subject.subscribe (rxjs/src/internal/Observable.ts:242:16)

Interestingly, these tests pass if I switch the unsubscribe call back to complete which had much more test failures. For reference, the code it's failing for looks something like this:

const currentPath = router.events.pipe(
  filter(event => event instanceof NavigationEnd),
  map(() => location.path()),
  startWith(location.path()),
);

[arturovt]

Seems like the subscription happens after the router is disposed (the root injector is destroyed)...

[crisbeto]

I've sent out a fix for the problematic test. Once it lands, I'll re-run all the tests to make sure that nothing else regressed.

[crisbeto]

Passing TGP

[crisbeto]

I'm bumping it to the minor branch just in case some external apps depend on subscribing after destroy.

[thePunderWoman]

This PR was merged into the repository by commit 52a6710.

The changes were merged into the following branches: main

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}