Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
thorn0 opened this issue Oct 12, 2014 · 6 comments
Open

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

thorn0 opened this issue Oct 12, 2014 · 6 comments
Milestone

Comments

@thorn0
Copy link
Contributor

thorn0 commented Oct 12, 2014

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
Copy link
Contributor

jeme commented Oct 13, 2014

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?...

@jeme jeme added this to the v0.7.0 milestone Oct 13, 2014
@thorn0
Copy link
Contributor Author

thorn0 commented Oct 13, 2014

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
Copy link
Contributor Author

thorn0 commented Oct 13, 2014

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
Copy link
Contributor

jeme commented Oct 13, 2014

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

@thorn0
Copy link
Contributor Author

thorn0 commented Oct 13, 2014

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
Copy link
Contributor

jeme commented Oct 14, 2014

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.

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

No branches or pull requests

2 participants