readline: add readline.promises #28407
Conversation
nodejs-github-bot
added
the
lib / src
addaleax
added
readline
cjihrig
changed the title
|
The documentation is ported. This is ready for review. |
|
Thanks @vsemozhetbyt. I've incorporated your suggestions. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Ping for reviews |
| createInterface, | ||
| cursorTo, | ||
| emitKeypressEvents, | ||
| moveCursor |
There was a problem hiding this comment.
I think all of these, except for Interface and emitKeypressEvents, are async operations because they write to the underlying stream. Should we account for that by making them return Promises?
There was a problem hiding this comment.
@addaleax I've been thinking about this. I'm a little torn. Returning a Promise does make sense. But why doesn't the current readline implementation consider the write() callback at all? The return value of write() is also not considered in either implementation currently.
There was a problem hiding this comment.
But why doesn't the current readline implementation consider the
write()callback at all? The return value ofwrite()is also not considered in either implementation currently.
Because inconsistent API design is one of Node’s big weaknesses, sadly… (And probably also because Node follows a “small writes work synchronously 99 % of the time” model, with all the pain that comes with the other 1 %).
But yeah, I’d prefer not repeating past mistakes. The return value is something I hadn’t considered yet, to be honest, but I’d probably give the write callback preference?
This commit moves lib/internal/readline.js to lib/internal/readline/utils.js. This is in preparation of adding a readline.promises implementation.
This commit moves the readline Interface class to its own file. All other readline logic is moved into the internal readline utils file.
This commit defines a named constant instead of using a mix of 2 ** 16 and 0X10000 throughout the code.
This changes the last two `var` uses in the readline code to `let`.
This commit refactors _tabComplete() to only pause the interface, call the completer() function, and then call another function to handle the results. This will make it simpler to support a promises based completer() function without duplicating much code.
This commit moves all Interface() constructor argument parsing into a helper function.
This commit exposes a Promise based version of the readline API.
Remove the readline.promises experimental warning per PR review comment.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/24360/ EDIT(cjihrig): CI was yellow. |
|
@cjihrig Was this closed intentionally? |
Yes. I want to do some work on the current |
This PR adds an experimental
readline.promises, similar to the ones in thednsandfsmodules. Thecompleter()argument to theInterfaceclass, and thequestion()are the primary differences from the callback based implementation.I also took this opportunity to freshen up the
readlinecode a bit (formatting comments, replacingvarwithlet/const, etc.).This is still WIP because I need to port the documentation.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes