-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Closed
Closed
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
c1613c0
internal: add {callback,promis}ify.js
chrisdickinson a8efd4f
zlib: promisify
chrisdickinson 4af7ec2
repl: promisify
chrisdickinson 0e0a2d9
readline: promisify .question + callbackify completer
chrisdickinson 8798a26
build: add callbackify and promisify to node.gyp
chrisdickinson 355205b
net: promisify socket.setTimeout; add connectAsync
chrisdickinson 7b8e57d
internal: fixup promisify
chrisdickinson 3f2b0d8
http,https: add {request,get}Async
chrisdickinson 23ba073
fs: no callback -> return promise
chrisdickinson 68837b8
dns: promisify
chrisdickinson 3317652
dgram: promisify .{bind,close,send}
chrisdickinson 846bd49
doc,dgram: add promise notes to dgram docs
chrisdickinson d90b42c
internal: add hooks for specifying different promise resolver
chrisdickinson adbe352
crypto: promisify pbkdf2, add randomBytesAsync
chrisdickinson 697ce01
child_process,cluster: promisify .send(), add exec{File,}Async
chrisdickinson f36b62d
add process.setPromiseImplementation
chrisdickinson 082fa08
process: add setPromiseImplementation API
chrisdickinson 54e3001
tls,net: connectAsync should resolve to Socket
chrisdickinson 014fb3e
lib: repromisify
chrisdickinson 15a42c1
repromisify
chrisdickinson 8586b10
src,lib: move back to native promises
chrisdickinson 40ca55e
domain,promise,src: add shim to make promises work with async_wrap + …
chrisdickinson 3928beb
domain: promises capture process.domain at .then time
chrisdickinson fa725b3
src: back asyncwrap integration out of this pr
chrisdickinson 0528d74
internal: lint
chrisdickinson 010224a
http{,s}: getAsync only listens for error once
chrisdickinson 6ae09c5
http{s,}: nix {request,get}Async
chrisdickinson 670a9c2
rework promisifier
chrisdickinson 240c72d
src: add flag
chrisdickinson 565dfe2
fix handler
chrisdickinson 3384ab0
introduce .promised
chrisdickinson 4e38057
bugfix: use outer arguments
chrisdickinson 8c97549
wip: add recovery object
chrisdickinson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Very uncomfortable with this kind of returns something or Promise kind of pattern. I would rather have a
sendAsync
kind of variant that always returns a Promise and keepsend
as it is. Overloading return values can get become problematic for developers who aren't careful.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.
Just to clarify, one example of how this could become tricky is that the return value on
child.send()
will be false if thewhen the backlog of unsent messages exceeds a threshold that makes it unwise to send more
, which means a developer can currently depend on the return value being falsy to help with throttling or to provide backpressure. If code is sending a bunch of messages and doesn't care about the callback, then a falsy check would fail...The fact that
callback
is already optional onsend
just makes this unpredictable.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.
A+, will change. This will likely influence how we approach this with streams as well, since the child process message functionality technically should be a stream.
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.
Is it common to node APIs or
process.send()
is an outlier? I don't remember ever using that return-and-callback-too pattern.Let me explain my concerns: "return a promise if no callback was provided" seems to be the least annoying way to support async/await. The other option is having a parallel API. Is
process.send()
the only thing that holds it back?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.
@gritzko The
crypto.randomBytes
function is already dual-mode - it will act synchronously without a callback, and asynchronously with a callback. While a different situation fromsend
, I'd imagine it would interfere in a similar way.