Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott commented Apr 1, 2022

No description provided.

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Apr 1, 2022
@TrottTrott removed doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Apr 1, 2022
bnb
bnb approved these changes Apr 1, 2022
Copy link
Contributor

@aduh95aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to recommend using util.promisify over the "built-in" promisified alternative.

Comment on lines 358 to 360
Use [`util.promisify()`][] to create a version that returns a
promise that fulfills with the answer. If the question is canceled using
an `AbortController` it will reject with an `AbortError`.
Copy link
Contributor

@aduh95aduh95Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Use[`util.promisify()`][] to create a version that returns a
promise that fulfills with the answer. If the question is canceled using
an `AbortController` it will reject with an `AbortError`.
Don't use[`util.promisify()`][] to create a version that returns a
promise that fulfills with the answer, use `require('readline/promises').createInterface` instead. If the question is canceled using
an `AbortController` it will reject with an `AbortError`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also recommend AbortSignal.timeout()

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's a documented promise API, I think we can just delete all the text, or replace it with something like "See [other API][] for a promise-based version of this API."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, removing it could make sense indeed. Worth noting that promise API is still marked as experimental and not available on Node.js <17, so we could expect a large chunk of our users would prefer to rely on util.promisify. Folks who want to support older release lines should check the documentation of the older release line.

@Trott
Copy link
MemberAuthor

Trott commented Apr 2, 2022

Content removed entirely. Does that work well enough, @aduh95?

@aduh95aduh95 added dont-land-on-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 2, 2022
@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-botnodejs-github-bot merged commit 059b890 into nodejs:masterApr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 059b890

@TrottTrott deleted the promisify branch April 3, 2022 05:04
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
PR-URL: nodejs#42552 Reviewed-By: Mestery <[email protected]> Reviewed-By: Tierney Cyren <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
This was referenced Apr 5, 2022
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
PR-URL: #42552 Reviewed-By: Mestery <[email protected]> Reviewed-By: Tierney Cyren <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
PR-URL: nodejs#42552 Reviewed-By: Mestery <[email protected]> Reviewed-By: Tierney Cyren <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Trott@nodejs-github-bot@bnb@targos@aduh95@Mesteery