Promises/A+ support #1141
Promises/A+ support #1141
Conversation
…:all is missing and there is no handling of return values from ->then callbacks or exceptions in ->then callbacks)
There was a problem hiding this comment.
Since I have little experience in the promise pattern I don't feel like I can review the new code very effectively. I am excited about the new capabilities especially if it can bring in new users.
That said, my concern about the change to catch still worries me greatly.
| sub begin { | ||
| my ($self, $offset, $len) = @_; | ||
| $self->{pending}++; | ||
| my $id = $self->{counter}++; | ||
| return sub { $self->_step($id, $offset // 1, $len, @_) }; | ||
| } | ||
|
|
||
| sub catch { shift->then(undef, shift) } |
There was a problem hiding this comment.
This breaks documented usage and can issue false negatives in common practice. I feel that to change this without a major version number increment or a deprecation is dangerous to our users. This could be mitigated by wrapping the passed in callback. Whether that be in a deprecation cycle or just a long-term fix is immaterial to me.
|
I disagree with @jberger and think documenting the possibility of breakage very prominently is good enough in this case. |
There was a problem hiding this comment.
Overall I'm positive to the change, but I think a major version and a note in the Changes file could be an idea. Another way to resolve this would be to make a new module and then deprecate both Mojo::IOLoop->delay and Mojo::IOLoop::Delay. Not too fond of that idea either, since a long(er) deprecation process should be in place for such a major change. I think this is tricky, but it could be resolved by saying that catch() is not well documented, tested and not widely in use on cpan, so let's just do the change.
| @@ -455,7 +467,7 @@ L<Mojo::IOLoop::Delay/"steps">. | |||
| say 'Never actually reached.'; | |||
| } | |||
| )->catch(sub { | |||
| my ($delay, $err) = @_; | |||
| my $err = shift; | |||
There was a problem hiding this comment.
Not very fond of this change, but it does not seem to be very many modules on CPAN that use catch(). (my cpangrep skills might be off of course)
There was a problem hiding this comment.
This is true, I did a similar search and by far the most users were me you and @Akron . Though I can imagine that delay/catch is used more in end-user code than in library code, so it might not matter that much.
| sub wait { | ||
| my $self = shift; | ||
| return if $self->ioloop->is_running; | ||
| $self->once(error => \&_die); |
lib/Mojo/IOLoop/Delay.pm
Outdated
| } | ||
|
|
||
| sub _die { $_[0]->has_subscribers('error') ? $_[0]->ioloop->stop : die $_[1] } | ||
|
|
||
| sub _settle { | ||
| my ($self, $status) = (shift, shift); | ||
| return $self if $self->{result}; |
There was a problem hiding this comment.
I read the js spec for promises and I think if already resolved, the reject() or resolve() methods should return undef and not $self.
http://www.ecma-international.org/ecma-262/7.0/index.html#sec-promise-resolve-functions
There was a problem hiding this comment.
Those two methods are not spec compliant by design. We can do there whatever has the most value for us.
There was a problem hiding this comment.
I wouldn't want this constructor just to be compatible.
Mojo::IOLoop::Delay->new(sub {
my ($resolve, $reject) = @_;
...
$resolve->('some value');
});There was a problem hiding this comment.
If you can find more use cases for boolean return values we can use those instead.
| @@ -81,7 +81,8 @@ sub _delay { | |||
| my $c = shift; | |||
| my $tx = $c->render_later->tx; | |||
| my $delay = Mojo::IOLoop->delay(@_); | |||
| $delay->catch(sub { $c->helpers->reply->exception(pop) and undef $tx })->wait; | |||
| $delay->on(error => sub { $c->helpers->reply->exception(pop) and undef $tx }); | |||
There was a problem hiding this comment.
Why do you need to change this line? Doesn't it work the same way, since you use pop and not $_[1] ?
There was a problem hiding this comment.
At the time i didn't want to change behavior, promise callbacks are not the same as event callbacks. But it might be worth changing if $c->delay is also used purely for promises.
Resolves #1139.