fix(router): remove RouterEvent from Event union type

[cexbrayat]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/main/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?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

41e2a68 added a type property on all router events, and added all type of events to the Event union type, but forgot to remove RouterEvent.
This removes the benefit of the type field, because it is not possible to write:

this.router.events.pipe(filter((event: Event): event is NavigationEnd => event.type === EventType.NavigationEnd)).subscribe(/*...*/);

as RouterEvent does not have a type field (hence TS complains).

What is the new behavior?

This commit fixes the issue by removing RouterEvent from the union type.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[atscott]

@cexbrayat This was actually done intentionally due to concerns around introducing a breaking change and time pressure with the v14 feature freeze. I've triggered internal presubmits to see how breaking this change really would be.

[atscott]

As expected, this is a breaking change. In particular, it would break this particular pattern to filter to the parent type (for example, if you want any event with an id or url):

            filter(
                (e: NavigationEvent): e is RouterEvent =>
                    e instanceof RouterEvent),

While we could consider not supporting this use-case in the same way, it's certainly not something we can just squeeze in to the v14 release since the RC is out and we can't introduce new breaking changes.

I'm going to close this since it's not feasible to land now. Could you open a feature request so we can gauge community interest and determine how much we should prioritize it for v15?

[cexbrayat]

@atscott I don't think it's particularly useful, it's just that the addition of type (and its intended goal) is not really usable right now. Let's keep it close then. Maybe it should just be removed from the changelog https://github.com/angular/angular/blob/main/CHANGELOG.md#1400-next16-2022-05-04 ?

[atscott]

@cexbrayat The intent was a resolution for #43762. Removing it from the changelog doesn't make sense because the change is merged and committed and we won't be reverting it. It's not as useful as it could be, but it's not entirely useless.

[clydin]

You should be able to use this form instead with the filter:
this.router.events.pipe(filter((event: Event): event is NavigationEnd => 'type' in event && event.type === EventType.NavigationEnd)).subscribe(/*...*/);

The need to hint that the filter function is type narrowing is unfortunately verbose in either case. It may be useful to introduce a series of helper functions (e.g., isNavigationEnd() or similar) in the future to more easily support the filter use cases.
This should reduce the code to something approaching:
this.router.events.pipe(filter(isNavigationEnd).subscribe(/*...*/);

[cexbrayat]

Thanks @clydin , that's what I'm currently using. I just thought leaving RouterEvent in the union type was unintended as the changelog led me to think 'type' in event would not be necessary 👍

Thanks @atscott for your time and explanation

[atscott]

@cexbrayat To be clear, I do think this is worth a feature request. You're absolutely right that the experience is not ideal at the moment.

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

[clydin]

@cexbrayat Would you be interesting in reopening and rebasing this change? We should hopefully be able to land this in v15.

[cexbrayat]

@clydin Sure, done!

[atscott]

@cexbrayat The team was really busy with a bunch of talks and conferences leading up to v15 so we didn't have time to do the internal cleanup necessary to land this before the feature freeze several weeks back. That said, we'll definitely get this in to v16. I'm doing the cleanup now and will add a patch to include this change so we don't have any regressions and will be able to merge as soon as the main branch is targeting v16.

That said, can you add the BREAKING CHANGE footer to your commit? Removing RouterEvent from the Event union type did result in a couple dozen build failures, mostly in tests with stubs. Sometimes this did extend to types used in production code, for example: filter((e: Event): e is RouterEvent => e instanceof RouterEvent) needed to change to filter((e: Event|RouterEvent): e is RouterEvent => e instanceof RouterEvent)

[cexbrayat]

@atscott Hi Andrew. No problem, I amended the commit, quoting your example as a use-case. Let me know if that's OK.

[atscott]

quick update: This change has been patched into Google's codebase. That means there will be no internal cleanup needed when we're ready to merge this to the v16 branch.

[atscott]

LGTM with one tiny non-code request: Can we update the commit message to describe the final result rather than the existing problem? i.e. "fix(router): Remove RouterEvent from Event type union to ..." or something to that effect

[atscott]

reviewed-for: public-api

[cexbrayat]

@atscott Sure, done 👍

[jessicajaniuk]

reviewed-for: public-api

[atscott]

merge assistance: This change is already patched in to g3. When syncing this commit, you'll need to also remove the router_events.patch (http://cl/510469344).

[alxhub]

This PR was merged into the repository by commit 1e32709.

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