fix(common): prevent duplicate URL change notifications

[LayZeeDK]

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using Location#onUrlChange.

PR Checklist

Please check if your PR fulfills the following requirements:

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

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

When two URL change listeners are registered, the first one will receive duplicate URL change notifications.

Issue Number: N/A

What is the new behavior?

When two URL change listeners are registered, both of them will only receive a single notification when the URL changes.

Does this PR introduce a breaking change?

• Yes
• No

Other information

Cc @aaronfrost

Location#onUrlChange is only used by NgUpgrade's $location shim for AngularJS.

[LayZeeDK]

Does it make sense to add an ngOnDestroy lifecycle hook to unsubscribe from the _onUrlChangeSubscription in the Location service? Would this make a difference to multiple applications on the same page? Is there a use case for applications being shut down after bootstrapping them? Hot Module Replacement maybe?

[JoostK]

Does it make sense to add an ngOnDestroy lifecycle hook to unsubscribe from the _onUrlChangeSubscription in the Location service? Would this make a difference to multiple applications on the same page? Is there a use case for applications being shut down after bootstrapping them? Hot Module Replacement maybe?

It looks like it won't be necessary to teardown the subscription manually, as the source is an EventEmitter that is local to the instance, and all referenced memory is also local to the instance. As soon as the instance is no longer referenced from other places, it will become eligible for the GC to collect.

[atscott]

👍 Thanks for this fix! Only some small nits in the comments

[atscott]

presubmit

[LayZeeDK]

Good to go.

I'll open another PR after merge to:

• Prefer private over @internal
• Prefer TestBed.inject over inject

in these files.

[LayZeeDK]

I think you have to remove the PR action: review label, @atscott 🙂

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