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

Provide an overload for $timeout that doesn't take a function #9176

Closed
benjamingr opened this Issue Sep 19, 2014 · 10 comments

Comments

Projects
None yet
5 participants
@benjamingr
Contributor

benjamingr commented Sep 19, 2014

I propose a $timeout variant that takes only a duration parameter.

There are two primary use cases I can think of

It would be really nice to be able to do:

$timeout(100).then(function(){
    // do stuff here, this runs a digest if it needs to which the
    // $timeout(function()... variant does not since 19b6b34
})

As well as

myServiceFn().then(function(res){
    // do something
    return $timeout(300);
}).then(function(){...

Which is rather common and today is accomplished by return $timeout(angular.noop, 300) which feels redundant and rather ugly.

This could be accomplished by adding a simple check here and I wouldn't mind creating a PR if you would entertain one.

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Sep 19, 2014

Member

This looks reasonable

Member

lgalfaso commented Sep 19, 2014

This looks reasonable

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Sep 19, 2014

Member

According to @caitp (#9175 (comment)) $timeout should still invoke $apply() if the invokeApply argument is not specified or truthy (if it doesn't then it is a bug and should be fixed).

If that is indeed the case, then I don't see this change making any difference.

Member

gkalpak commented Sep 19, 2014

According to @caitp (#9175 (comment)) $timeout should still invoke $apply() if the invokeApply argument is not specified or truthy (if it doesn't then it is a bug and should be fixed).

If that is indeed the case, then I don't see this change making any difference.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 19, 2014

Contributor

yes, I'm going to see if I can reproduce what they're talking about, because this was tested manually before

Contributor

caitp commented Sep 19, 2014

yes, I'm going to see if I can reproduce what they're talking about, because this was tested manually before

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 19, 2014

Contributor

http://jsbin.com/tesaqovopaza/1/edit can't reproduce at all

Contributor

caitp commented Sep 19, 2014

http://jsbin.com/tesaqovopaza/1/edit can't reproduce at all

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 19, 2014

Contributor

Wait hang on, these things are talking about two very different issues ^_^

Contributor

caitp commented Sep 19, 2014

Wait hang on, these things are talking about two very different issues ^_^

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Sep 19, 2014

Member

@caitp:
What I said is that if the other issue is not an issue (i.e. if $timeout is working as expected), then the modification proposed in this issue is not particularly valuable.

I.e. everything that will be possible using $timeout(100).then(callback) is already possible using $timeout(callback, 100). (It's even less code with the current implementation.)

Member

gkalpak commented Sep 19, 2014

@caitp:
What I said is that if the other issue is not an issue (i.e. if $timeout is working as expected), then the modification proposed in this issue is not particularly valuable.

I.e. everything that will be possible using $timeout(100).then(callback) is already possible using $timeout(callback, 100). (It's even less code with the current implementation.)

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 19, 2014

Contributor

yeah, I sort of agree that it's a weird feature to add, but if people really want it, it probably won't account for much code

Contributor

caitp commented Sep 19, 2014

yeah, I sort of agree that it's a weird feature to add, but if people really want it, it probably won't account for much code

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 19, 2014

Contributor

@caitp here is a fiddle illustrating it working after a .then -http://jsfiddle.net/c8rchefp/ here is it not assimilating a promise without a .then http://jsfiddle.net/x4y3kjzf/ which is not what I'd expect.

After a second check this is not the breaking change since promises returning promises don't schedule a digest at the current moment anyway (although that's not the behavior I'd expect).

I'm still all for this feature though for the promise chain use case regardless of this. I'll close the PR on the other issue, my bad.

Contributor

benjamingr commented Sep 19, 2014

@caitp here is a fiddle illustrating it working after a .then -http://jsfiddle.net/c8rchefp/ here is it not assimilating a promise without a .then http://jsfiddle.net/x4y3kjzf/ which is not what I'd expect.

After a second check this is not the breaking change since promises returning promises don't schedule a digest at the current moment anyway (although that's not the behavior I'd expect).

I'm still all for this feature though for the promise chain use case regardless of this. I'll close the PR on the other issue, my bad.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Sep 19, 2014

Contributor

http://jsfiddle.net/3n9akpdg/ --- works this way. It looks like something is wrong with the way we're dealing with external promises, you should file a bug on $q

Contributor

caitp commented Sep 19, 2014

http://jsfiddle.net/3n9akpdg/ --- works this way. It looks like something is wrong with the way we're dealing with external promises, you should file a bug on $q

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 19, 2014

Contributor

@caitp it's not a bug since the digest system is completely orthogonal to the concerns of the Promises/A+ specification. Looking back at how $q behaves in 1.2 this is not a change. The overload is still nice for the promise chain case.

I'll write a PR for this in the next few days and accompanying tests.

Contributor

benjamingr commented Sep 19, 2014

@caitp it's not a bug since the digest system is completely orthogonal to the concerns of the Promises/A+ specification. Looking back at how $q behaves in 1.2 this is not a change. The overload is still nice for the promise chain case.

I'll write a PR for this in the next few days and accompanying tests.

@btford btford added this to the Backlog milestone Sep 19, 2014

vasileorza added a commit to vasileorza/angular.js that referenced this issue Oct 21, 2014

feat($timeout): overload service API.
Provide an overload for $timeout that doesn't take a function.

Closes #9176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment