Skip to content
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

Added promise support to `rl.question` in the readline module. #28010

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@MilesZew
Copy link

commented Jun 1, 2019

As described in the first commit: added a promise as the return value to the Interface.prototype.question function in the readline module. Inside that promise I placed the resolve and reject functions on the _resolveQuestionPromise and _rejectQuestionPromise properties respectively. Ran the _resolveQuestionPromise in the Interface.prototype._onLine function if the _resolveQuestionPromise function existed, the argument being the line argument passed into the Interface.prototype._onLine function.

I made a simple documentation change explaining that rl.question can now be used with async/await or .then syntax:

rl.question can also be used with async/await and .then

async function askQuestion(question) {
  const answer = await rl.question(question);
  console.log(`Thank you for your valuable feedback: ${answer}`);
  rl.close();
}

The other commits were me experimenting with my forked repo and deleting and auto generated file and are not additional features or changes.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@MilesZew MilesZew marked this pull request as ready for review Jun 1, 2019

@MilesZew MilesZew force-pushed the MilesZew:MilesZew-1 branch 2 times, most recently from 7c80a51 to 39c7df1 Jun 1, 2019

MilesZew added some commits Jun 1, 2019

readline: added promise support to `rl.question`
In the readline module, In the `Interface.prototype.question` function
I added a promise return value. The Promise places the resolve and
reject functions on the `_resolveQuestionPromise`
and `_rejectQuestionPromise` properties respectivly.
The resolve function is then call in the
`Interface.prototype._onLine` if it (the resolve function) exists.

Added these lines to the readline.md:
`rl.question` can also be used with `async`/`await` and `.then`

```js
async function askQuestion(question) {
  const answer = await rl.question(question);
  console.log(`Thank you for your valuable feedback: ${answer}`);
  rl.close();
}
```

@MilesZew MilesZew force-pushed the MilesZew:MilesZew-1 branch from 39c7df1 to a1e8f89 Jun 2, 2019

@Himself65

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

related issue #27864

@Himself65
Copy link
Contributor

left a comment

I think need a unit test🤔

@@ -301,6 +303,11 @@ Interface.prototype.question = function(query, cb) {
this.prompt();
}
}

return new Promise((resolve, reject) => {

This comment has been minimized.

Copy link
@Himself65

Himself65 Jun 2, 2019

Contributor

I think only return Promise when no parameter cb.

This comment has been minimized.

Copy link
@MilesZew

MilesZew Jun 2, 2019

Author

This is probably a good idea, although there could be some edge cases where both are needed. I can only think of one reason to keep it, and that would be for slightly shortening code:

const someFunction = require('somefunction')

async someAsyncFunction() {
    const answer = await question('What do you think of node.js')
    console.log(`thank you for your feedback: ${answer}`)
    someFunction(answer)
}

//becomes

async someAsyncFunction() {
    someFunction(await question('What do you think of node.js', answer => console.log(`thank you for your feedback: ${answer}`)))
}

It's probably better the first way, but some people might want to use it the second way.

This comment has been minimized.

Copy link
@Himself65

Himself65 Jun 2, 2019

Contributor

but in a second way, we don't know which will run firstly. This example must be a nightmare

someFunction1(await question('What do you think of node.js', answer => someFunciton2()))
@MilesZew

This comment has been minimized.

Copy link
Author

commented Jun 2, 2019

I think need a unit test🤔

I couldn't find anything documenting how to create unit tests for node, and did not want to create a test without knowing what I should do first as I am a first time contributor.

@Himself65

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

@MilesZew
in my view, add a file named such as test-readline-async-question.js on test/parallel folder

and learn other test files🤔

@cjihrig
Copy link
Contributor

left a comment

Can we avoid introducing promises into legacy callback APIs?

@MilesZew

This comment has been minimized.

Copy link
Author

commented Jun 2, 2019

Can we avoid introducing promises into legacy callback APIs?
@cjihrig

We could create a separate api similar to the fs.promises api, this feature has been requested and I personally prefer promises over callbacks.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

I'm not opposed to this, and I'd personally prefer to await question(). A new API is fine. I'd just prefer not to introduce this into a legacy callback API because some users (mostly postmortem debugging users) prefer not to introduce promises into production code just by updating their node version.

@Himself65

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

I'm not opposed to this, and I'd personally prefer to await question(). A new API is fine. I'd just prefer not to introduce this into a legacy callback API because some users (mostly postmortem debugging users) prefer not to introduce promises into production code just by updating their node version.

so, check the length of arguments.length to handle callback(cb) or return a promise?

@MilesZew

This comment has been minimized.

Copy link
Author

commented Jun 2, 2019

I'm not opposed to this, and I'd personally prefer to await question(). A new API is fine. ...

In that case I could change the code to look like this?

...
Interface.prototype.promises.question = function(query, cb) {
...

or maybe I could just check for a variable:

...
if(exports.promises) {
    return new Promise((resolve, reject) => {
    self._resolveQuestionPromise = resolve;
    self._rejectQuestionPromise = reject;
  });
}
...
@Himself65

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

I looked up the dns.js.

this would be better which create a file named lib/internal/dns/promises.js, and code promise.question.

because there will must have more Promise API later

example:

node/lib/dns.js

Lines 320 to 332 in 187b056

Object.defineProperties(module.exports, {
promises: {
configurable: true,
enumerable: true,
get() {
if (promises === null) {
promises = require('internal/dns/promises');
promises.setServers = defaultResolverSetServers;
}
return promises;
}
}
});

@Trott

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

I think need a unit test🤔

I couldn't find anything documenting how to create unit tests for node, and did not want to create a test without knowing what I should do first as I am a first time contributor.

https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

@BridgeAR
Copy link
Member

left a comment

Since such a promise can not reject at all, I wonder if it makes sense to add a default timeout and add an option to pass through individual timeouts in case no callback is used. Ideally, such timeout would also be used in combination with the callback but it seems like we have an inconsistency here with our default callback style because it only has an answer argument and no error argument. Adding an error argument as first argument would be a significant breaking change and I do not see that it's possible to do that in this case.


return new Promise((resolve, reject) => {
self._resolveQuestionPromise = resolve;
self._rejectQuestionPromise = reject;

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jun 4, 2019

Member

It is possible to use this instead of self due to the usage of the array function. The _rejectQuestionPromise is also not used at all in the code. But we do not have to attach any new variables to the instance. Instead, it's possible to create an own internal callback that resolves the promise as in:

Interface.prototype.question = function(query, cb) {
  let promise;
  if (typeof cb !== 'function') {
    if (cb !== undefined) {
      throw new ERR_INVALID_ARG_TYPE(...);
    }
    promise = new Promise((resolve) => {
      const tmpCallback = this._questionCallback;
      if (tmpCallback) {
        this._questionCallback = (answer) => {
          tmpCallback(answer);
          resolve(answer);
        };
      } else {
        cb = resolve;
      }
    });
  }
  if (this._questionCallback) {
    this.prompt();
  } else {
    this._oldPrompt = this._prompt;
    this.setPrompt(query);
    this._questionCallback = cb;
    this.prompt();
  }
  return promise;
};
@@ -301,6 +303,11 @@ Interface.prototype.question = function(query, cb) {
this.prompt();
}
}

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jun 4, 2019

Member

This would probably turn the change into a semver-major change but I would verify that callback is either a function or undefined and throw an error if it's anything else.

@MilesZew

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

I am closing this pull request In order to work on a new API, better error handling, better documentation, etc.

@MilesZew MilesZew closed this Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.