-
-
Save mjackson/fafef431758f45ba3b830ad9cbeb2328 to your computer and use it in GitHub Desktop.
| /** | |
| * Registers the given callback to be called in the node.js callback | |
| * style, with error as the first argument, when the promise resolves. | |
| * | |
| * Also, returns a function that may be used to prevent the callback | |
| * from ever being called. Calling the returned function is synonymous | |
| * with saying "I no longer care about the resolution of this promise, | |
| * even if it fails." | |
| * | |
| * Since there is no provision in the promise spec for cancel/abort | |
| * behavior, this may be used as a workaround. | |
| * | |
| * In React components, you can use this function as a workaround for | |
| * the deprecation of the isMounted() feature. | |
| * | |
| * const AsyncInput = React.createClass({ | |
| * handleResponse(error, value) { | |
| * this.setState(...) | |
| * }, | |
| * handleChange(event) { | |
| * this.ignoreResponse = createBinding( | |
| * makeRequest(event.target.value), | |
| * this.handleResponse | |
| * ) | |
| * }, | |
| * componentWillUnmount() { | |
| * // Ignore any requests that are currently in progress. | |
| * if (this.ignoreResponse) | |
| * this.ignoreResponse() | |
| * }, | |
| * render() { | |
| * return <input onChange={this.handleChange}/> | |
| * } | |
| * }) | |
| */ | |
| export const createBinding = (promise, callback) => { | |
| let isIgnored = false | |
| promise.then( | |
| value => !isIgnored && callback(null, value), | |
| error => !isIgnored && callback(error) | |
| ) | |
| return () => | |
| isIgnored = true | |
| } |
I disagree, @rnicholus. A lot of people share this idea (I've heard it a few times before) so please allow me to expand a little on why I disagree.
tl;dr: Our job here is to handle errors thrown by the promise, not the callback.
The whole promise paradigm is designed to model JavaScript's native try/catch behavior with an asynchronous call "stack". For purposes of illustration, let's assume that promise is a synchronous operation, modeled with a function that either returns or throws. If this were the case, my version of createBinding would look something like this:
let value
let isError = false
try {
value = promise()
} catch (error) {
isError = true
value = error
}
if (isError) {
callback(value)
} else {
callback(null, value)
}This is fairly straightforward: try to call callback with the return value of the operation, but catch the error and use it as the first argument if the operation fails. One key characteristic of this code is that it makes no attempt to catch any errors that callback itself might throw. That's not its job. It's only supposed to resolve the promised value.
In the version you propose, with the second .catch chained onto the end of the first .then, the sync equivalent would be:
try {
callback(null, promise())
} catch (error) {
callback(error)
}The .catch in your example essentially says "catch everything, both errors in promise() and in callback()". We now have the following problems:
- Inside the
catchblock, how do you know where theerrorcame from? Was it thrown bypromise()? Or somewhere insidecallback()? - If
callbackis the culprit:- How can we know it won't just
throwagain when we call it in thecatchblock? - Is it safe to call
callbacktwice? We're calling it for the 2nd time inside thecatch.
- How can we know it won't just
These are semantics that we need to communicate to consumers if we're going to catch errors that callback throws.
Probably a nitpick, but it seems the "rejected" callback function passed to
thenis sub-optimal to registering acatchhandler instead. The benefit ofcatchover passing a second callback function to yourthencall is thatcatchwill also be called if the "resolve" callback throws. So that results in the following adjustment tocreateBinding:A nitpick since it arguably isn't important for a canned example like this, but it seems
catchis a best practice, so perhaps it should be promoted in examples.