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

Return a promise from $location.path (and search) to detect if the change was cancelled #7392

Open
Shazwazza opened this issue May 8, 2014 · 17 comments

Comments

@Shazwazza
Copy link

Currently if you do a $location.path("/blah") (with optional .search) you have no way of knowing if someone has cancelled your attempt at changing the path by using a listener on $locationChangeStart + event.preventDefault()

Currently there is no event for $locationChangeFailed so it's near impossible to know if you path change attempt was cancelled without some hackery or custom events that are emitted in the code that is calling event.preventDefault().

@Shazwazza Shazwazza changed the title Return a promise from $location.path (and search) to detect if the change is successful Return a promise from $location.path (and search) to detect if the change was cancelled May 8, 2014
@pkozlowski-opensource pkozlowski-opensource added this to the Backlog milestone May 8, 2014
@caitp
Copy link
Contributor

caitp commented May 8, 2014

How exactly would a location change fail? It doesn't make an http request or anything, so its not really clear

@pkozlowski-opensource
Copy link
Member

@caitp I guess this is about the fact that the location change can be canceled. So it doesn't fail but can be prevented. It is true that in this case we are not propagating any location change error:
https://github.com/angular/angular.js/blob/master/src/ng/location.js#L738

@caitp
Copy link
Contributor

caitp commented May 8, 2014

Yeah but nothing in core cancels it, and presumably if a user cancels it, they know what they're doing. If a module cancels it, they might get another way, like $routeChangeError (although ngRoute only cares about $locationChangeSuccess) --- It doesn't hurt to add another event, but that codepath is really rare/never happens

@Shazwazza
Copy link
Author

Unfortunately route change error doesn't fire when it's cancelled either.

Sent from my phone

@caitp
Copy link
Contributor

caitp commented May 8, 2014

Because nothing ever cancels it, basically (it's really a situation where if you cancel it, you know that you cancelled it)

@Shazwazza
Copy link
Author

What I'm saying is that if something cancels the location change, there is
no way to know. The route change error event does not fire, hence the
reason for logging this issue.

The problem is that we have a very large app, some ui gets shown, a user
clicks on a button,and tries to change the location. Another panel cancels
the location change because a form is dirty, the panel that the user
clicked the button on needs to close.

I've solved this by raising a custom event so the code that tried to change
the location knows that it was cancelled. Just seems that of the
location.path could return a promise as one of its properties to let the
caller know if it was successful or not would be more ideal. Or if the
route failed event fired, that'd work as well but is less ideal.

Sent from my phone

@caitp
Copy link
Contributor

caitp commented May 8, 2014

@Shandem the other thing is that this is not an asynchronous operation at all, so a promise really doesn't make sense there

@Shazwazza
Copy link
Author

But it must be async somewhere in there... If you set location.path the
logic continues but the cancelling op in the location change start event
happens async, or at least that code to cancel in the event handler does
not execute directly after the call to change location.

Sent from my phone

@caitp
Copy link
Contributor

caitp commented May 8, 2014

The time between $locationChangeStart and $locationChangeSuccess is entirely synchronous.

@Shazwazza
Copy link
Author

Sure, but what about between the operating of actually setting
location.path and location change start?

Sent from my phone

@caitp
Copy link
Contributor

caitp commented May 8, 2014

@Shandem well, here's what happens. If the browser supports the history API, we're listening for navigation events (like popstate or hashchange) -- and if it doesn't, we're pretty much polling to see when the location changes (as well as intercepting clicks within the angular app). So, by the time we actually know that location changed, or the browser url has changed, it's a pretty much synchronous operation

@Shazwazza
Copy link
Author

Sorry didn't get time to put a snippet together for you today but this is
what happens when doing this:

// In module B:

$scope.$on("$locationChangeStart", function(event, newPath, oldPath) {
// Check for something
if ("something" === "something")
// cancel location change
event.preventDefault();
});
});

// In module A:

var result = $location.path("/blah");
//If this were running non-async I should be able to do something like this:
if (result.cancelled) {

}

But instead the execution continues in module A before the event handler
executes in module B which means the location changing is async to the
current execution. So without a promise returned, there would be no way to
wait in the current execution to know the result. An event could be raised
like $locationChangeCancelled instead which would also work but means you'd
have to write more code to deal with that (inevitably probably creating a
method that does this for you and returns a promise)

Shannon Deminick
w: http://shazwazza.com http://shazwazza.com/
e: [email protected]
s: shannondeminick
t: shazwazza

On Fri, May 9, 2014 at 1:53 AM, Caitlin Potter [email protected]:

@Shandem https://github.com/Shandem well, here's what happens. If the
browser supports the history API, we're listening for navigation events
(like popstate or hashchange) -- and if it doesn't, we're pretty much
polling to see when the location changes (as well as intercepting clicks
within the angular app). So, by the time we actually know that locationchanged, or the browser url has changed, it's a pretty much synchronous
operation


Reply to this email directly or view it on GitHubhttps://github.com//issues/7392#issuecomment-42567179
.

@caitp
Copy link
Contributor

caitp commented May 9, 2014

$location.path(...) doesn't change window.location immediately, this is true. it waits until the next digest, from which point the whole operation is synchronous. $location.path() is also a synchronous operation, it just doesn't include changing window.location, rather it specifies an intent to change window.location.

It wouldn't make sense to return a promise from $location.path() or any of the other getter/setters there, really. Just handle $routeChangeStart and cancel the location change if you want to, notify other areas that you cancelled via scope events. no biggy, and you want to know about this synchronously

@Shazwazza
Copy link
Author

Righto, no worries then, that's basically what I've done

Sent from my phone

@btford btford removed the gh: issue label Aug 20, 2014
@zachsnow
Copy link
Contributor

$locationChangeCancelled would be useful when the part of the application that is canceling the location change is a library / reusable directive / service / whatever. Otherwise each place that cancels the location change might trigger its own event, and then you either hope that everywhere uses the same event, or you end up handling several.

(Obviously if the entire application is under your control it's easy to always trigger your own event. You can also work around it with e.wasDefaultPrevented. But it would be convenient.)

@honkskillet
Copy link

It would be nice if $location.path('someUrl') would return a promise. If the $routeProvide resolve is rejected, then the promise would be rejected.

location.path('/userSettings')
  .then(function(){
  //success
  },function(error){
     alert(error);//handles the rejection
  }

and

angular.module('myApp')
  .config(function ($routeProvider) {
    $routeProvider
      .when('/userSettings', {
        templateUrl: 'app/routes/userSettings/userSettings.html',
        controller: 'UserSettingsCtrl',
        resolve:{
          authenticatedUser: function(Auth){
            return Auth.getAuthenticatedUserAsync(); //returns a promise
          },
        },
      });
  });

@litera
Copy link

litera commented Oct 9, 2015

@honkskillet if $location.path would return a promise it still wouldn't help much, because you wouldn't have any chance of rejecting it after your $routeChangeStart event would execute. The problem is that URL is already changed when your route gets to the $routeChangeSuccess or $routeChangeError. Especially the last one should have the ability to reject location change promise.

Second problem is, who would resolve it if nobody actually cared about it? How should it work then? If it issued a promise that you could resolve/reject, but nobody did that?

The major change that we could introduce to $location.path() is to make it asynchronous.

The problem is this cycle:

  1. location change start even fires
  2. route change start event fires
  3. location change success fires (this is when URL in your browser has already changed)
  4. route starts preparing route resolves and waits for all promises to resolve
  5. promises resolved, route change success fires

All is well even thought step 3 should fire after step 5.

Now the main problem is when step 5 reads like this: at least one route.resolve promise fails which fires route change error event.

Why is this a problem? Because URL has already changed to the new URL even though routing didn't get that far. So we end up with an old view with the new URL. Which is clearly wrong.

And with route.resolve promises anything can cause their rejection. Network failure, user permissions on server... you name it... Too many things really.

So there is clearly a bug that's also outlined in #2100 (not resolved since beginning of 2013 when it was reported!!!). And that's the reason why $location.path should be rewritten somehow to support asynchronous processing. We also have to consider that $location.path should work just fine even if we didn't use ngRoute at all. Or any other router for that matter.

@caitp I hope you understand why synchronous nature (or just invalid execution of $location.path) is messing things up here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants