Skip to content
This repository was 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$locationChangeStart handler called outside of $apply/$digest Behavior of $location.path() inconsistent when back and forward buttons are used
3 participants