dotJEM / angular-routing

Enhanced state based routing for Angular applications!
https://dotjem.github.io/angular-routing/
MIT License
75 stars 9 forks source link

Use of 'return' inside of 'finally' callbacks #122

Open thorn0 opened 9 years ago

thorn0 commented 9 years ago

I'm trying to tackle an issue in my project with impossibility to reload the state and constantly getting Transition defered by another call to goto. I can't isolate it so far. However, I started looking into the code of the router and came across a probable reason.

See this line: state.ts, line 678. The finally method is used here apparently with an assumption that returning a value from it has some effect. However, the docs state (emphasis mine) the opposite:

finally(callback) – allows you to observe either the fulfillment or rejection of a promise, but to do so without modifying the final value. This is useful to release resources or do some clean-up that needs to be done whether the promise was rejected or resolved. See the full specification for more information.

Thus there is no point in assignment like gotoPromise = gotoPromise.finally(...). It does nothing. The same with: gotoPromise.finally(function() { /*...*/ return ...; }). This return is useless, the returned value goes to nowhere.

jeme commented 9 years ago

If we look at the code (https://github.com/angular/angular.js/blob/master/src/ng/q.js#L288) for finally, it actually just delegates to using both the success and error handler in then. Meaning that both the parts you describe are important as they ensure that the finally handler has been run before the next step.

However, it seems we can't recover from an error state inside finally (the value we return will be retained, but it's state does not change)...

The initial rational was that the caller of goto (or reload etc.) could actually use the returned promise to see if that goto or reload completed successfully or with an error.

But as this is bubbling out into the console if one does not handle it, and that an aborted transition is not to be seen as uncommon, perhaps forcing a recovery of the promise but returning an informational object would be a better approach:

E.g.

gotoPromise = gotoPromise.finally(...)
   .then(function() { return SUCCESS_OBJ; }, function() { return FAIL_OBJ; });

But I doubt very much that that is the specific issue in your case if something also doesn't appear to be working?

Is it possible to make a Plunker with the issue?...

thorn0 commented 9 years ago

Yes, I'm trying to isolate it into a plunker, but without any success for now. I'm integrating an Angular (1.3-rc5) app into a legacy application (non-Angular) that already has code that uses the HTML5 history API. And the issue I'm fighting with is that $state.reload(true) stops working after history.pushState was called by the legacy part.

thorn0 commented 9 years ago

I replaced this line (route.ts, line 663):

$rootScope.$on(EVENTS.LOCATION_CHANGE, function () { promise.then(update); });

with

var savedUrl;
$rootScope.$on(EVENTS.LOCATION_CHANGE, function () {
    if (typeof savedUrl == 'undefined') {
        savedUrl = $location.url();
    } else {
        if ($location.url() === savedUrl) {
            return;
        }
        savedUrl = $location.url();
    }
    promise.then(update);
});

And it started to work. Apparently it's related to the latest changes to $location in Angular 1.3. Now it tracks not only the URL, but also the HTML5 history state object.

jeme commented 9 years ago

Ok... ill mark it with AngularJS 1.3 and have a look one of the following days.

thorn0 commented 9 years ago

I'm sorry for such low quality of this issue. I mean absence of the plunker demo etc. For now I'm plain happy that everything started to work, but I used this code in a project, and it means that I'll have to return to this issue one day and give it another closer look.

jeme commented 9 years ago

That is ok, the whole idea behind providing a plunker with it reproduces is to speed things up, its a "help me help you" thing, I am not going to just discard issues that doesn't have a plunker like the UI-Router team almost does...

But it will get lower priority until sufficient information is available... In your case you choose to debug away instead, and that way provide some more information and that gives me more meat to work with, again making it easier for me to again help you with the issue (e.g. fixing it)...

So all in all I am happy that respect that and do what you can to isolate it one way or another.