Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($location): ensure $locationChangeStart broadcast during $digest #5580

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Dec 31, 2013

I'm not totally convinced this is a necessary change (see comments on #5118).

However, feedback valuable. I'll amend this message if it becomes clear that
it's a worthwhile change.

Closes #5118
Closes #4989

I'm not totally convinced this is a necessary change (see comments on angular#5118).

However, feedback valuable. I'll amend this message if it becomes clear that
it's a worthwhile change.
@ghost ghost assigned tbosch and IgorMinar Jan 3, 2014
@IgorMinar
Copy link
Contributor

this is also related to #4989

@caitp
Copy link
Contributor Author

caitp commented Jan 3, 2014

The issue is that the $browser calls fireUrlChange iterating over each of the listeners without wrapping in a $digest --- the effect is that the event is emitted outside of a digest.

The reason I say this change is sort of questionable is because it's not clear that it really matters in any significant fashion, but I suppose it's better to not force people to check $$phase

So we do fire a digest later, but this is after the event is already handled, and it's all unrelated to the if statement

@caitp
Copy link
Contributor Author

caitp commented Jan 3, 2014

Hmm, I guess maybe I favour #5089 over this, after re-thinking it

@IgorMinar
Copy link
Contributor

I'm thinking that we should do both this and #5089.

@caitp
Copy link
Contributor Author

caitp commented Jan 3, 2014

I see ... well in any case, both PRs are missing tests, I'll write some up tomorrow I guess

@IgorMinar
Copy link
Contributor

I'll add tests and merge them both today. I want to be done with this issue :)

@caitp
Copy link
Contributor Author

caitp commented Jan 3, 2014

fair enough, PS happy newyear \o/

@IgorMinar
Copy link
Contributor

I just realized that by moving the broadcast into evalAsync, we will always execute it within a $digest phase, so this PR is not necessary.

I think we'll get rid of the $phase check and double $digest error in 1.3 (we talked about in the office today and I think I know how to do it), so with that change this $phase check will disappear and we'll always wrap the whole thing into $apply without causing double digest error.

@IgorMinar
Copy link
Contributor

oh, yeah. happy new year! :)

@IgorMinar IgorMinar closed this Jan 3, 2014
IgorMinar pushed a commit that referenced this pull request Jan 3, 2014
…is triggered by the browser

Fixed inconsistency in $location.path() behaviour on the $locationChangeStart event when using
back/forward buttons in the browser or manually changing the url in the address bar.
$location.path() now returns the target url in these cases.

Closes #4989
Closes #5089
Closes #5118
Closes #5580
@caitp caitp deleted the issue-5118 branch January 16, 2014 03:43
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…is triggered by the browser

Fixed inconsistency in $location.path() behaviour on the $locationChangeStart event when using
back/forward buttons in the browser or manually changing the url in the address bar.
$location.path() now returns the target url in these cases.

Closes angular#4989
Closes angular#5089
Closes angular#5118
Closes angular#5580
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
…is triggered by the browser

Fixed inconsistency in $location.path() behaviour on the $locationChangeStart event when using
back/forward buttons in the browser or manually changing the url in the address bar.
$location.path() now returns the target url in these cases.

Closes angular#4989
Closes angular#5089
Closes angular#5118
Closes angular#5580
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.