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

Documents API changes in #256 #946

Closed
wants to merge 1 commit into from
Closed

Documents API changes in #256 #946

wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

Documents the new unhandled rejection detection API.

Awesome work on that PR everyone!

#256 (comment)

// application specific logging, throwing an error, or other logic here
});

## Event: 'rejectionDetected'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rejectionHandled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkurchatkin Yes, I wrote this before the name changed - nice catch

@domenic
Copy link
Contributor

domenic commented Feb 25, 2015

I disagree with the framing here and think it is biased. I can comment more when back at my computer (which will be later tonight).

@benjamingr
Copy link
Member Author

@domenic biased against what? I tried to keep it as simple as possible and using the least promise terminology. Is your issue with the terminology or are you looking for a more 'pro-promise' sentiment?

@petkaantonov
Copy link
Contributor

I think it should simply cover how every philosophy is implemented and their pros and cons in an unbiased way, for example not saying that something is rare but saying that in some programming styles this will result in false positives.

@petkaantonov
Copy link
Contributor

There doesn't seem to be a way to document either event in isolation without being biased actually.

Any usage example of unhandledRejection event in isolation is biased towards instant logging/throwing philosophy, because what else can you do in there if you use that event alone?

And rejectionHandled is only needed if you buy into the growing and shrinking list of rejections philosophy so documenting its usage is going to be biased towards that obviously..

@benjamingr
Copy link
Member Author

@petkaantonov which is why this PR after the update due to @domenic 's comment shows both patterns. The unhandledRejection event is demonstrated in isolation and then in the rejectionHandled event the shrinking list idea is explained (paraphrasing @domenic's own words) and demonstrated. I think the current balance is ok - anything you'd change?

@petkaantonov
Copy link
Contributor

Your update doesn't include any example usage of the 'unhandledRejection' event, it just says that an event will be fired but you don't show example code handling it.

@benjamingr
Copy link
Member Author

@petkaantonov from the doc:


Here is an example that logs every unhandled rejection to the console:

process.on('unhandledRejection', function(reason, p){
    console.log("Possibly Unhandled Rejection at: Promise ", p, " reason: ", reason);
    // application specific logging, throwing an error, or other logic here
});

@rvagg rvagg mentioned this pull request Feb 25, 2015
@@ -116,6 +116,45 @@ Nine out of ten times nothing happens - but the 10th time, your system is bust.

You have been warned.

## Event: 'unhandledRejection'

Emitted whenever a `Promise` is rejected and no error handler is attached to the promise within a turn of the event loop. When programming with promises exceptions are encapsulated as rejected promises. Such promises can be caught and handled using `promise.catch(...)` and rejections are propagated through a promise chain. This event is useful for detecting and keeping track of promises that were rejected whose rejections were not handled yet. This event is emitted with the following arguments:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "rejection handler" would be better than "error handler" but nbd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic I think rejection handler is more concise promise terminology but I chose error handler since I found it more approachable for people without a background with promises - if you feel strongly about this I can change it though.

@domenic
Copy link
Contributor

domenic commented Feb 25, 2015

OK. I've left extensive comments on ways to improve this, because I am passionate about this subject getting good docs treatment. HOWEVER, @benjamingr's edits since my original comment have addressed the bias I perceived, and so I think if there's a release imminent it's fine to merge this as-is without necessarily implementing my changes. We should do them eventually though since they're good changes :)

// application specific logging, throwing an error, or other logic here
});

For example, here is a rejection that will be trigger the `"unhandledRejection"` event:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"will trigger" not "will be trigger"

@sam-github
Copy link
Contributor

Events in markdown should use single quotes, for example 'rejectionHandled'.

The example uses an odd mix of double and single quotes for strings, I'd suggest single quotes everywhere.

Markdown should be wrapped, like all other text lines in io.js.

@benjamingr
Copy link
Member Author

Thanks @domenic and @sam-github I've updated the PR with all the advice and the formatting issues included except for the FileHandle example since I've found it a bit controversial (the API format itself) and I think we can come up with a more concise one. I've pretty much merged all the proposed changes by domenic (good terminology remarks) - the only thing I didn't change there explicitly was "error handler" to "rejection handler" because I find the terminology more approachable to people who are new to promises.

@benjamingr
Copy link
Member Author

Rebased.

## Event: 'rejectionHandled'

Emitted whenever a Promise was rejected and an error handler was attached to it
later than after an event loop turn. This event is emitted with the following
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is better to say: after after 'unhandledRejection' was emitted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkurchatkin I considered it but figured it's better to phrase it containing when it emits as a whole (after a promise was rejected and no handler was attached in time). I wanted to add a 'this happens when an "unhanldedRejection" event was emitted' but figured that it's better to give an example of using them together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @vkurchatkin

Emitted whenever a Promise was rejected and an error handler was attached to it later than after an event loop turn.

Sounds like "emitted when the 'unhandledRejection' has a listener added".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 that's what I'm trying to avoid - you don't have to add a listener to the "unhandledRejection" event in order for this event to fire. You might be interested in using this event for purposes that are not related to that other one. Even if you are - I think the semantics of this event should stand up for themselves.

Maybe adding it as an extra? How would you phrase it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe more like:

"Emitted whenever a Promise was previously rejected and an error handler (such as .catch()) was attached to it"

Documents the new unhandled rejection detection API.

Documents the new unhandledRejection/rejectionHandled events in the process
docuemntation. As agreed on in this issue:

#256 (comment)

PR-URL: #946
Reviewed-By: Domenic Denicola <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@rvagg
Copy link
Member

rvagg commented Feb 25, 2015

OK, I want to get a release out today and this is going to get merged to make that happen. If anybody disagrees with the language please suggest changes, otherwise perhaps reserve them for a future PR.

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

Successfully merging this pull request may close these issues.

8 participants