feat($location): allow to change location during $locationChangeStart

[shahata]

Closes #9607

[shahata]

@tbosch - While I'm afraid adding custom methods to scope events is currently not a mechanism that $rootScope enables, I believe this can be fixed by simply testing if the location was changed while the scope event was processed. So basically, all the developer needs to do is call one of the $location setters in the event handler in order to redirect to somewhere else (no need to call preventDefault in that case, it has no effect)

WDYT?

[gkalpak]

This line (and a few more beloew) is too long (> 100 characters)
:(

[caitp]

what I'd like to see is a case where a nested $locationChangeStart handler prevents default. if that case works right, then it's cool with me

[shahata]

@caitp I've added the test case you requested

[petebacondarwin]

Assigning to @pkozlowski-opensource as he is working on #9607

[pkozlowski-opensource]

Looking at this PR right now and so far so good but I wonder if we shouldn't have a test where the $locationChangeStart is triggered by the routing system. Normally we should be fine, but since the original issue was opened for the routing, maybe we should play it on the safe side?

[pkozlowski-opensource]

OK, went over this patch once again and it LGTM based on my (limited) knowledge of the $location code. Having said this I would love to see 2 more tests:

• The current implementation creates an interesting corner case where the location change start event can be both prevented and someone might change the $location's URL - we should make it clear what happens in this case
• As noted above, a test on the router level, something along those lines:

  it('should allow redirects while handling $routeChangeStart', function () {
    module(function($routeProvider) {
      $routeProvider.when('/some', {
        id: 'some', template: 'Some functionality'
      });
      $routeProvider.when('/redirect', {
        id: 'redirect'
      });
    });
    module(provideLog);
    inject(function($route, $location, $rootScope, $compile, log) {
      $rootScope.$on('$routeChangeStart', function(event, next, current) {
        if (next.id === 'some') {
          $location.path('/redirect');
        }
      });
      $compile('<div><div ng-view></div></div>')($rootScope);
      $rootScope.$on('$routeChangeStart', log.fn('routeChangeStart'));
      $rootScope.$on('$routeChangeError', log.fn('routeChangeError'));
      $rootScope.$on('$routeChangeSuccess', log.fn('routeChangeSuccess'));
      $rootScope.$apply(function() {
        $location.path('/some');
      });

      expect($route.current.id).toBe('redirect');
      expect($location.path()).toBe('/redirect');
      expect(log).toEqual([ 'routeChangeStart', 'routeChangeStart', 'routeChangeSuccess']);
    });
  });

As noted above I'm not too familiar with the $location code / design so it would be awesome if someone with more experience in this area could have another look. @caitp @petebacondarwin @lgalfaso anyone?

[shahata]

The current implementation creates an interesting corner case where the location change start event can be both prevented and someone might change the $location's URL - we should make it clear what happens in this case

@pkozlowski-opensource

I don't think there's a corner case here, if $location is changed, we will be redirected to that new address - It doesn't matter if you prevented the initial location change or not, it will have no effect. Or did you mean something else?

I added the router test you suggested, thanks.

[pkozlowski-opensource]

@shahata

I don't think there's a corner case here, if $location is changed, we will be redirected to that new address - It doesn't matter if you prevented the initial location change or not, it will have no effect.

This is what I've meant - that is - we should explicitly say what is a behaviour if an event is prevented and redirection happens while processing this event. This is why I was suggesting leaving a note in the code or better yet - a test - that makes this explicit.

[shahata]

@pkozlowski-opensource okay, added comment and test

[pkozlowski-opensource]

Great @shahata !

[pkozlowski-opensource]

@caitp @petebacondarwin this one LGTM now but it would be great to have another pair of eyes looking

[caitp]

i feel like it would look weird if we emit $routeChangeStart without a matching $routeChangeError or $routeChangeSuccess --- do you think this will cause issues for people?

[shahata]

Doing the current workaround as suggested in the original issue will result in the exact same event sequence, so I don't see a problem with this...

The only difference is that instead of doing:

$rootScope.$on("$routeChangeStart", function (event, next, current) {
  if (next && next.secure) {
    event.preventDefault();
    $rootScope.$evalAsync(function() {
      $location.path('/c');
    });
  }
});

You would do this:

$rootScope.$on("$routeChangeStart", function (event, next, current) {
  if (next && next.secure) {
    $location.path('/c');
  }
});

[shahata]

Is there anything preventing us from merging this?

[petebacondarwin]

Just adding my eyes now!

[petebacondarwin]

These comments don't quite read right to me... should they instead be:

// only process the location change if the location Url was not changed by a
// `$locationChangeStart` event handler.

[shahata]

done