If the injector is already destroyed, that error is still caught by the catch block so I'm not sure why this would really be necessary. Presumably everything's shutting down anyways, so what issues does this cause?

Edit: This change also isn't quite equivalent to the previous state because you're missing the NavigationError event as well as resolving or rejecting the navigation promise at the very least.

The issue is that runInInjectionContext throws an error that injector has already been destroyed and that's getting logged by our services (which practically shouldn't).

If the application is already destroyed, the catch block should not execute anything in practice (because other stuff has already been released and destroyed).

If the application is already destroyed, the catch block should not execute anything in practice (because other stuff has already been released and destroyed).

Okay, yea that's fair. I would reframe the justification for this. It's not exactly that we don't want to do runInInjectionContext but really that we don't want to do anything at all. If we're destroyed, we shouldn't do more work, regardless of what that is (and the error really isn't an error, it's just comes that way because of the way rxjs is working here).

That said, I'm still not sure whether we should omit the event and the promise completion and only return EMPTY.

Alright, maybe we should wrap the code with if !destroyed, but still resolve/reject the promise? (It would still return empty at the end).

Btw, the _events is unsubscribe()d on dispose (see this._events.unsubscribe()), so there's no reason to emit events as there will be no observers.

Alright, maybe we should wrap the code with if !destroyed, but still resolve/reject the promise? (It would still return empty at the end).

Yea, let's do overallTransitionState.resolve(false); return EMPTY;.

Btw, the _events is unsubscribe()d on dispose (see this._events.unsubscribe()), so there's no reason to emit events as there will be no observers.

👍

👍 will update the code tomorrow.

@atscott updated the code to include resolve(false).

FYI, removing the "merge" label for now, since we'd need to run a global presubmit for this change. We'll re-add the "merge" label back if the presubmit is successful.

TGP is green.

This PR was merged into the repository by commit c7b6e11.

The changes were merged into the following branches: main, 19.1.x

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.}