In reference to https://github.com/sabre-io/event/pull/66#67
In reference to https://github.com/sabre-io/event/pull/66#67TheTechsTech wants to merge 23 commits intosabre-io:masterfrom
Conversation
…act, Guzzle, AMP.
…ssed by reference
… why some tests in NotWorkingPromiseTest .php still fails
… general test file for Promise compatibility, mark failing as skipped.
|
Thank you, I'm going to go through these and see what's up! |
evert
left a comment
evert
left a comment
There was a problem hiding this comment.
In short, i see 1 actionable change here:
- Add a
resolveand alias + deprecate thefulfillfunction. I think this is sensible if other projects use the same function name.
The others can be categorized as:
- Executor is running synchronously, like JS
- Rejecting with non-exceptions
- Adding cancel support
I disagree with adding support for 1 & 2. I can see that it might be useful for some to have a default implementation of a CancelablePromise. However, I am curious what your specific use-case for this is? Do you intend to use this library and this is a must-have for you? It's the first time this request was made. I feel that this can be largely implement with just ->reject but maybe I'm missing something important.
| { | ||
| $finalValue = 0; | ||
| $promise = new Promise(); | ||
| $promise->resolve(1); |
There was a problem hiding this comment.
I didn't realize, but this first failure is simply because the ->resolve() function doesn't exist. This library uses ->fulfill. I am OK with creating a new function resolve() and make the fulfill() function alias to this. This seems like a good change.
This change should be its own independent pull request. We can mark fulfill() as deprecated and remove it in some future version.
| $p->then(null, null) | ||
| ->then(null, function ($v) use (&$r) { $r = $v; return $v . '2'; }) | ||
| ->then(function ($v) use (&$r2) { $r2 = $v; }); | ||
| $p->reject('foo'); |
There was a problem hiding this comment.
The second problem is that reject() is called with a string. I don't support this change. Every error should be stay an instance of throwable.
I realize that Javascript does allow rejecting with strings, but in javacript it's also possible to throw 'foo';. I think it's more appropriate for PHP to restrict rejecting strictly with throwable.
| public function testThrowsWhenUnwrapIsRejectedWithException() | ||
| { | ||
| $e = new \Exception('foo'); | ||
| $p = new Promise(function () use (&$p, $e) { $p->reject($e); }); |
There was a problem hiding this comment.
I don't think it's possible for this test to pass. When $p->reject is called, $p doesn't have a value yet. The promise executor should be executed synchronously, just like the Javascript Promise.
There was a problem hiding this comment.
There's actually a lot of tests that have this issue.
| } | ||
|
|
||
| public function testRejectsSelfWhenWaitThrows() | ||
| { |
There was a problem hiding this comment.
I disagree with this test. An exception thrown in the executor function should not result in a rejected promise, it should immediately fail. This is the same as the javascript promise object.
You missed my first reply. I was checking to find the issues in my own promise library, which i have done. I started here cause ReactPHP have an series of promise tests that all mocks. and your promise implementation operate similar and have real tests and different from Guzzle. |
|
Ah I see, that's clear. I think I would say then that adding "cancel" support is probably not something we should do unless there's someone out there that will want to use it ;) |
2 participants
The issues i'm have when adding some promise tests that works elsewhere.