Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(common): update $locationShim to notify onChange listeners before… #32037

Closed
wants to merge 1 commit into from

Conversation

@agale123
Copy link
Contributor

commented Aug 7, 2019

… emitting AngularJS events

The $locationShim has onChange listeners to allow for synchronization logic between
AngularJS and Angular. When the AngularJS routing events are emitted first, this can
cause Angular code to be out of sync. Notifying the listeners earlier solves the
problem.

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Listeners to the $locationShim onChange are notified after the $locationChangeSuccess or $locationChangeStart events.

Issue Number: N/A

What is the new behavior?

Listeners to the $locationShim onChange are notified immediately before the $locationChangeSuccess or $locationChangeStart events.

Does this PR introduce a breaking change?

  • Yes
  • No

If someone is already using this method and depending on the existing ordering of events then technically this could be breaking. But the contract for the onChange method doesn't make any guarentees about the ordering of these events, it just says that the listeners are notified.

Other information

See b/138246666 for more details on the Google internal bug causing this change.

fix(common): update $locationShim to notify onChange listeners before…
… emitting AngularJS events

The $locationShim has onChange listeners to allow for synchronization logic between
AngularJS and Angular. When the AngularJS routing events are emitted first, this can
cause Angular code to be out of sync. Notifying the listeners earlier solves the
problem.

@agale123 agale123 requested a review from angular/fw-upgrade as a code owner Aug 7, 2019

@googlebot googlebot added the cla: yes label Aug 7, 2019

@petebacondarwin
Copy link
Member

left a comment

Seems reasonable to me. Thanks @agale123

@agale123

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@kara kara closed this in 5064dc7 Aug 13, 2019

sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(common): update $locationShim to notify onChange listeners before…
… emitting AngularJS events (angular#32037)

The $locationShim has onChange listeners to allow for synchronization logic between
AngularJS and Angular. When the AngularJS routing events are emitted first, this can
cause Angular code to be out of sync. Notifying the listeners earlier solves the
problem.

PR Close angular#32037
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Sep 15, 2019

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.