readline: add Promise-based API

[aduh95]

This needs tests, but I'd like to get some feedback first to see if it's a direction we want to go at all.

• provides a refactored version of readline.Interface written as ES6 class.
• provides promesified version of callback based methods of readline.

semver-major because it exposes a new core module readline/promises.

Fixes: #37287

//cc @nodejs/readline

[Linkgoron]

Should the async question throw if the interface is closed (otherwise an await might hang forever, if I understand the code correctly)? Also, I assume that this is a legacy issue with the way that the callback was written, but should it throw if it failed to write?

[aduh95]

Should the async question throw if the interface is closed (otherwise an await might hang forever, if I understand the code correctly)? Also, I assume that this is a legacy issue with the way that the callback was written, but should it throw if it failed to write?

My gut instinct would be to not try to fix it, and instead encourage users add a timeout if a never resolving promise is an issue for them.

[benjamingr]

Generally +1 on the idea, I haven’t tested the code in front of a computer but just wanted to prove early “this is interesting to me” feedback.

[aduh95]

@nodejs/tsc @nodejs/readline this is ready for reviews.

[targos]

What makes the PR semver-major?

[aduh95]

What makes the PR semver-major?

It adds a new core module (readline/promises) and with the refactoring to ES6 class, I think it's safer to land this in a semver-major.

[targos]

Was the refactoring necessary to make the promise version?
This pull request is difficult to review because it does everything in one commit and moves a lot of code. That's also a problem for backporting conflicts.

[aduh95]

The refactoring is necessary if we don't want the promise version to expose its internal methods and properties. I can certainly split this is in multiple commits to help the review process though.

[mcollina]

I do not think the mapping to the promises is correct in this case as it is losing an important use case: it is less usable than the callback counterpart.

[mcollina]

(I'm responding to #37947 (comment) in the main issue because I think it's relevant to everybody)

The goal of introducing promise-based version of our API is to deprecate the callback version: it must be functionally equivalent.

At the current state this PR it is not equivalent. Consider the following:

// The errors bubble up naturally 
cursorTo(stream, 0, 1)
cursorTo(stream, 2, 3)

vs

// Await is needed to catch errors
await cursorTo(stream, 0, 1)
await cursorTo(stream, 2, 3)

Those two API are not equivalent because the promise version waits for the chunk to be fully written before allowing the new command.

In reality, what would be needed to make them equivalent is:

await Promise.all([
  await cursorTo(stream, 0, 1)
  await cursorTo(stream, 2, 3)
])

I think it's a far worse user experience that the callback API we aim to replace.

[aduh95]

In reality, what would be needed to make them equivalent is:

await Promise.all([
  await cursorTo(stream, 0, 1)
  await cursorTo(stream, 2, 3)
])

I think it's a far worse user experience that the callback API we aim to replace.

I think a more equivalent code would look more something like:

await new Promise(async (resolve, reject) => {
  cursorTo(stream, 0, 1).catch(reject);
  cursorTo(stream, 2, 3).catch(reject);

  // rest of the logic
  // …
  // …

  resolve();
});

I'm afraid it's just a limitation of how promises work in JS, I don't think there's much we can do without changing the language or breaking some user expectations. Currently, I believe no promise-based core API throws synchronously on invalid arguments (E.G.: fs.promises.readFile(Symbol()) returns a rejected promise), and I firmly believe we should aim for consistency.

Maybe we can settle on adding an option argument for users who don't like the default behaviour? Something like { throwOnInvalidArguments: true, returnStreamDrainInfo: true }.

[benjamingr]

Why not pass the cursorTo error to the stream instead of rejecting the promise with it?

I think your example with:

  cursorTo(stream, 0, 1).catch(reject);
  cursorTo(stream, 2, 3).catch(reject);

Shows Matteo's point of how problematic this API can be (since the second reject would be swallowed if the promise above it would be executed)

[aduh95]

Landed in 0f1765e...341312d

[ljharb]

@aduh95 can you confirm what node version you expect this to land in? (and/or ping me if it's going to be backported to previous majors?)

[aduh95]

It's labeled semver-major, meaning it won't land on any of the current release lines. It should be part on v17.0.0 though.

[targos]

Are we sure about the API? Maybe we should make it experimental in case we need to make changes after it's released?