-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
Adding Core support for Promises #5020
Changes from 1 commit
c1613c0
a8efd4f
4af7ec2
0e0a2d9
8798a26
355205b
7b8e57d
3f2b0d8
23ba073
68837b8
3317652
846bd49
d90b42c
adbe352
697ce01
f36b62d
082fa08
54e3001
014fb3e
15a42c1
8586b10
40ca55e
3928beb
fa725b3
0528d74
010224a
6ae09c5
670a9c2
240c72d
565dfe2
3384ab0
4e38057
8c97549
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,23 +11,35 @@ module.exports = { | |
}; | ||
|
||
function normal(object, name) { | ||
return _promisify(object, name, (err, val) => { | ||
return _promisify(object, name, (lastArg, err, val) => { | ||
if (err) { | ||
if (lastArg && | ||
typeof lastArg === 'object' && | ||
err.code && | ||
typeof lastArg[err.code] === 'function') { | ||
return lastArg[err.code]() | ||
} | ||
throw err; | ||
} | ||
return val; | ||
}); | ||
} | ||
|
||
function single(object, name) { | ||
return _promisify(object, name, (val) => { | ||
return _promisify(object, name, (lastArg, val) => { | ||
return val; | ||
}); | ||
} | ||
|
||
function multiple(object, name, mapped) { | ||
return _promisify(object, name, (err, arg0, arg1) => { | ||
return _promisify(object, name, (lastArg, err, arg0, arg1) => { | ||
if (err) { | ||
if (lastArg && | ||
typeof lastArg === 'object' && | ||
err.code && | ||
typeof lastArg[err.code] === 'function') { | ||
return lastArg[err.code]() | ||
} | ||
throw err; | ||
} | ||
return { | ||
|
@@ -42,12 +54,13 @@ function _promisify(object, name, handler) { | |
|
||
function inner() { | ||
const args = Array.from(arguments); | ||
const lastArg = args[args.length - 1]; | ||
return new Promise((resolve, reject) => { | ||
fn.call(this, ...args, resolver); | ||
|
||
function resolver(arg0, arg1, arg2) { | ||
try { | ||
resolve(handler(arg0, arg1, arg2)); | ||
resolve(handler(lastArg, arg0, arg1, arg2)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reminds me of cases where I do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jfhbrook yes, promises normalize that - if you pass |
||
} catch (err) { | ||
reject(err); | ||
} | ||
|
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.
Isn't this optimization killer?
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.
yes, to avoid it copy the arguments manually with a for loop.
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.
Function can be already deopted because of
try-catch
below, so ¯_(ツ)_/¯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, that probably needs moving into a specialist
attempt
function which only does the try...catch and returns the result.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.
Or we can just not care about those kinds of optimizations as function is already just a wrapper for another one :) (and V8 compilers change over time, too)
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.
Seems like it's not an issue anymore with the latest V8: https://twitter.com/kdzwinel/status/667078434598682624
And this is exactly what bothers me: it's used as a wrapper for all promised API. But, as someone pointed above, IO is so extremely slow that this deopt may not affect overall performance. So more likely you're right, but it would be nice to do at least some basic benchmarking. Just in case 😉
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.
That's why I said "compilers change over time" (but, as far as I remember, they co-exist now and try-catch will be fast only under certain conditions). Anyway, great to see progress - and with more engines coming, I'm pretty sure we don't need to care neither about deopts nor about "we will slow down callbacks if check whether they're passed or not" - this is I/O after all.