-
Notifications
You must be signed in to change notification settings - Fork 11
Conversation
Fixes #11. |
To complement the producer defined operational/programmer errors, the consumer defined unexpected/expected errors should be added as well |
|
||
A function passed to `Promise#then` or `Promise#catch`. Receives as an argument | ||
the parent promise's resolved value or rejected error. Represents a new | ||
`Promise`. May return a value `T` or `Promise<T>`, which resolves the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already duplicates the Settling section a bit, but might I suggest:
A handler which returns a value
T
orPromise<T>
will resolve the new promise withT
. One which throws an errorR
, or returns aPromise
which itself eventually rejects (or has already rejected) withR
, will reject the new promise withR
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Added verbatim.
|
||
```js | ||
const p1 = new Promise(() => { throw new Error('xyz')}) | ||
const p2.catch(() => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean p2 = p1.catch...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Thanks!
> be avoided by changing the code. They can never be handled properly (since by | ||
> definition the code in question is broken). | ||
|
||
By definition, these errors are unexpected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth mentioning that these errors are not always synchronous and that operational errors are not always asynchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this definition describes error states, but not their propagation. I'll add a note stating that this section pointedly doesn't mentioned propagation types, and note the recommendations of the error symposium.
const p = new Promise(() => {}) | ||
|
||
const p2 = p.then(function SuccessHandler(value) { | ||
// fired if p resolves, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"fulfills"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
I left some comments. I also think this is missing a lot of terms from the issue but that can be handled later at a later PR. |
@benjamingr: Thank you for the thorough review. How would you feel about merging this as-is, then fixing up the promises section with subsequent PRs? |
|
||
```js | ||
const p1 = new Promise(() => { throw new Error('xyz')}) | ||
const p2 = p1.catch(() => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not:
const p1 = new Promise(() => { throw new Error('xyz')});
const p2 = p1.catch((err) => {
// err === [Error: xyz]
});
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Thanks!
|
||
```js | ||
const p1 = new Promise(() => { throw new Error('xyz')}) | ||
const p2 = p1.catch(() => { /* err === [Error: xyz] */ }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
() => { /* err === [Error: xyz] */ }
should be
(err) => { /* err === [Error: xyz] */ }
;-)
I think another iteration would be useful - and after another rounds of comments we can gladly merge. |
Thank you for the review. In the interest of freeing others up to work on this in subsequent PRs (and to free myself to work on the other required bits!), I'm going to merge this. |
I strongly object the definition you used for unexpected/expected errors, let's please work it out before merging. See my inline comment |
* [Handler](#handler) | ||
* [Settling](#settling) | ||
* [Rejection / reject](#rejection--reject) | ||
* [Resolving / resolve](#resolving--resolve) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it useful to organize this as:
- promise states: pending or settled
- settled states: fulfilled or rejected
- defer (to create a pending promise, or to resolve a pending promise with another pending promise)
- resolve (to take a pending promise and, having made some progress, to designate another promise (albeit a fulfilled, rejected, or pending promise) as its resolution, responsible for eventually notifying its observers when it is ultimately settled)
- fulfill (to create a fulfilled promise, or resolve and settle a pending promise with a fulfilled promise, eventually notifying all of its eventual handlers of the fulfillment value)
- reject (to create a rejected promise, or resolve and settle a pending promise with a rejected promise, eventually notifying all of its eventual handlers of the rejection error)
This defines common terms for discussion.
/cc'ing @misterdjules, @petkaantonov, @kriskowal for review.