globals: introduce `prompt` on global #38552

Ayase-252 · 2021-05-05T10:42:40Z

Background

When writing a CLI application on Node.js, there is a pain point when ask user for input. Without third-party module, currently the best way to ask user input is to use readline module.

However, it could be somehow "too complex" for some simple use case(like CLI-based guessing number game or just simply ask people to choose a project name in a script).

const readline = require('readline');

const rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout
});

rl.question('What do you think of Node.js? ', (answer) => {
  console.log(`Thank you for your valuable feedback: ${answer}`);
  rl.close();
});

In this PR, it adds a new global function prompt, which accepts a message and defaultValue. It echoes message to user, and returns user input synchronously. Example is shown as following

> prompt("hello world")
hello world
hello
'hello'

API Design Consideration

The origin issue suggested something like readline.input or process.input. Thanks to @artembykov's suggestion, I think prompt on global would be ideal solution to given problem. prompt method is standardzied in browser side, and also is implemented on global in deno. Introducing prompt will not make API in JS world scattering, while benifits user.

Fixes: #37925

Fixes: nodejs#37925

nodejs-github-bot · 2021-05-05T13:18:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37925/

aduh95

I personally don't think it's a good idea to implement a sync API for this – I get the point regarding web compatibility, but async would make much more sense for this feature IMHO.

I'm adding a request change to reflect my personal -1, and to make sure this doesn't land too quickly; the topic of introducing legacy web APIs probably deserves a discussion as a whole and maybe a decision from the TSC.

aduh95 · 2021-05-05T13:31:22Z

.eslintrc.js

@@ -333,5 +333,6 @@ module.exports = {
    btoa: 'readable',
    atob: 'readable',
    performance: 'readable',
+    prompt: 'readable'


Suggested change

prompt: 'readable'

prompt: 'readable',

aduh95 · 2021-05-05T13:32:51Z

lib/internal/bootstrap/node.js

@@ -246,6 +247,60 @@ if (!config.noBrowserGlobals) {
    return performance;
  });

+  defineOperation(globalThis, 'prompt', function prompt(
+    message = 'prompt',
+    defaultValue


Suggested change

defaultValue

defaultValue = null

aduh95 · 2021-05-05T13:32:58Z

lib/internal/bootstrap/node.js

+      validateString(defaultValue);
+    }
+
+    defaultValue ??= null;


Suggested change

defaultValue ??= null;

aduh95 · 2021-05-05T13:34:20Z

lib/internal/bootstrap/node.js

+        if (charaBuf[0] === LF) {
+          break;
+        }
+        res.push(charaBuf[0]);


Suggested change

res.push(charaBuf[0]);

ArrayPrototypePush(res, charaBuf[0]);

jasnell · 2021-05-05T14:25:06Z

I'd be +1 on this. I get the concerns about it not being async but the point about web compat is sound. One thing we might also consider is opening a proposal upstream to add an async version of the prompt and confirm APIs to the html spec.

jasnell · 2021-05-05T14:27:01Z

doc/api/globals.md

+returns `null`.
+If default value is given and the user's input is empty string, it returns
+`defaultValue`.
+Otherwise, it returns the user's input as `string`.


Should describe in here whether this method blocks the progression of the event loop or not.

mscdex

-1 Things like this are easily implemented in userland (there are even await-able examples in the readline docs currently) and don't belong in node core IMO (especially as a global).

jasnell · 2021-05-05T14:28:52Z

test/pseudo-tty/test-prompt.js

+const common = require('../common');
+const assert = require('assert');
+
+(async () => {


There's no reason for this to be wrapped in an async function.

jasnell · 2021-05-05T14:49:42Z

... and don't belong in node core IMO (especially as a global)

FWIW, I disagree entirely. Where such functions exist across multiple platforms and runtimes, and where there is similar functionality already included, it makes sense to conform to the standard API vs. continuing to rely on a non-standard Node.js specific API.

jasnell · 2021-05-05T14:52:58Z

lib/internal/bootstrap/node.js

+        fs.openSync('/dev/tty', 'r');
+      const res = [];
+
+      while (true) {


My preference would be to implement this in terms of the functionality already available in readline and repl if possible. Also, burying this implementation into internal/bootstrap/node.js makes it more difficult to find and maintain, especially given that it differs from the similar functionality in readline.

The use of sync fs methods can also be problematic here.

I'm +1 on the idea of getting this into core but I don't think the impl as it is now is the right way.

Maybe it could go into tty.js – maybe as a method on the tty.ReadStream prototype, so it can be used like this:

const answer = await process.stdin.prompt('Question?\n');

I'd try to implement it on the top of readline/repl.

VoltrexKeyva · 2021-05-05T15:33:05Z

lib/internal/bootstrap/node.js

+    // Mimic implementation of `prompt` of deno
+    function readFromStdInSync() {
+      const fs = require('fs');
+      const LF = '\n'.charCodeAt(0);


Suggested change

const LF = '\n'.charCodeAt(0);

const LF = StringPrototypeCharCodeAt('\n', 0);

Use primordials for consistency.

VoltrexKeyva · 2021-05-05T15:33:55Z

lib/internal/bootstrap/node.js

+    function readFromStdInSync() {
+      const fs = require('fs');
+      const LF = '\n'.charCodeAt(0);
+      const CR = '\r'.charCodeAt(0);


Suggested change

const CR = '\r'.charCodeAt(0);

const CR = StringPrototypeCharCodeAt('\r', 0);

Same case as above.

addaleax

I also don't think we should implement user-facing web APIs, so I'm -1 as well.

Web compatibility is a nice thing for libraries, because they are made to be reusable, so I can see why we add stuff like atob that should not be used at all, but compatibility of UI code is way out of scope for Node.js, imo. People are just not going to write (non-trivial) applications that run in both Node.js and the browser, and if they really want to, then I agree with @mscdex that that is decidedly a problem for userland to solve.

(I also agree in that the readline APIs are better for this purpose.)

addaleax · 2021-05-05T23:09:33Z

lib/internal/bootstrap/node.js

+          break;
+        }
+        if (charaBuf[0] === CR) {
+          const bytesRead = fs.readSync(stdfd, charaBuf, 0, 1);


This kind of implementation is going to be inherently flaky in cases in which process.stdin is also used for reading.

devsnek · 2021-05-05T23:13:08Z

if we add this can we also add blocking xhr?

jasnell · 2021-05-05T23:31:00Z

@addaleax:

but compatibility of UI code is way out of scope for Node.js, imo

I'd agree if Node.js did not already include UI code that could have been easily implemented as userland code. If the intent is that UI code in Node.js really ought to be the realm of userland and the module ecosystem, then perhaps what we really ought to be doing is de-emphasizing userland use of readline and repl as reusable APIs. (Not saying deprecate or remove them, but mark them legacy and stop evolving them)

addaleax · 2021-05-06T09:51:17Z

@jasnell Yes, I would love it if we could separate out readline and/or REPL, put them into their own npm packages, and just vendor them in or something like that for our internal usage and encourage people to use the npm packages (the way we handled node-inspect up until recently, basically).

(This is not just something that I think would be theoretically nice enough to have – it would also be useful just for my day-to-day work, and probably enough so that I might be able to some company time allocated just to help make that happen. It’s somewhat ouf of scope for this PR discussion here, but getting a feeling for whether @nodejs/tsc would be interested in this at all would be nice.)

Ayase-252 · 2021-05-06T12:31:04Z

I'd separate my rationale into two questions, that

Should this functionality be implemented in core?
Should the functionality be implemented as API as given?

For question 1, I can’t come up with a reason why not to implement it in core.

Reading from stdin is fairly common usage of Node.js. When writing small (not so advanced) script, it would be handy to have a one-line method, which doesn’t require user to consider heavy async/promisify/callback to do that.
Also, it benifits beginners of JS, Since Promise/Closure are advanced topics for them, it lowers barrier for JS beginner to create interactive program in Node.js.
Most programming languages have such functionality on their built-in or standard library. It demonstrates reading from stdin sync is a fundamental demand. For JavaScript, Node.js is the ad-hoc standard library, IMO. Given that readline-sync has 604K downloads/w, I think we can safely assume that people demand such functionality eagerly.

For question 2, While I think global.prompt is the best place if we implements, since it is an existing function. We could borrow its semantic from the spec, I’m also open to alternative option, such as a static method in readline or tty like readline.prompt/ tty.prompt, or a method on process, like process.prompt.

…don't belong in node core IMO (especially as a global).

While it is true that users can easily(?) implement interactive CLI application with current facilities, it needs a lot of boilerplate code. It is unintuitive for beginner and cumbersome for even advanced users.

I also don't think we should implement user-facing web APIs….

if we add this can we also add blocking xhr?

I understand that Node.js has fundamental difference with Browser. Some Web APIs wouldn't make sense or deems as “legacy” in the context of Node.js. But my first intention is to facilitate one of the most common tasks. It seems that I want to make Node.js more web-compat by introducing more Web APIs. Really, I don't mean that. That is a consequence but not a intention.

addaleax · 2021-05-06T12:36:29Z

But my first intention is to facilitate one of the most common tasks.

I think the best way to achieve that for this specific feature would be better documentation, e.g. not bury the await rl.question() example in the last part of https://nodejs.org/api/readline.html#readline_rl_question_query_options_callback, and instead highlight it at the beginning of the readline docs, for example, or generally move the example up in the docs.

mcollina

I'm ok in adding some method to easily prompt the user.

I'm -1 in adding a synchronous, global method to do so.

Considering top-level await, the following is quite a nice API:

import { prompt } from 'tty'

await prompt('hello?')

addaleax · 2021-05-06T12:58:12Z

Yeah, I think @mcollina’s suggestion would be a bit nicer. (Also, just from an API design perspective, this should be built on top of readline and accept at least a subset of readline’s options, i.e. input and output stream, whether to treat the input stream as a terminal, etc. – And generally, I think the readline module would be a better fit than the tty one, but that’s details.)

Ayase-252 · 2021-05-06T13:29:05Z

@mcollina @addaleax

Thanks for suggestion, it looks nice to me too as described in my alternative options.

I'd plan to leave the PR open for a while to gather concerns about the functionality before drafting a new PR.

jasnell · 2021-05-06T13:32:02Z

Implementing as an async option with additional options gets us further from web compat but what we could do here is design the api with the intention of proposing it in upstream standards also.

anirudhgiri · 2021-05-06T13:45:58Z

Author of the original issue here. First of all, thank you for taking the time to address the issue I raised, I was expecting it to be ignored but to see that this community cares about issues raised by its users this much really is heartwarming.

I'm seeing some resistance to the idea of having an API for this at all by some people, with the argument being "Just make the user use the readline module and rl.question() if they really want user input from stdin", so allow me to put in my two cents.

As time goes on, Javascript+NodeJS is a combination that is moving from being a tool that is used exclusively for developing serverside applications for the web to more of a general-purpose programming language like C, Python, and Java are. JS has gone from a "programming language for web-related stuff" to being used to write GUIs, Operating Systems, and even machine learning models. Like it or not, Javascript is going to become a completely general-purpose language that is taught to children in schools one day, and if we want that to happen we really need to lower the barrier of entry for beginners.

Prompting the user for input from stdin is such a fundamental feature for any programming language that it is one of the first things that are taught to beginners. Python has input() and C has scanf() -- simple one-line methods but to get user input with Node on the other hand is a chore that requires a whole lot of boilerplate and knowledge about first-class functions and how callbacks work.

This is why I believe that it is imperative to include a method to easily prompt the user for input. As for the details of how it should be implemented is up to you guys. I would personally prefer if it were a synchronous and global function (like Python's input() or Deno's prompt()).

aduh95 · 2021-05-07T13:33:32Z

test/pseudo-tty/test-prompt.in

@@ -0,0 +1,2 @@
+sushi


Can you test with non-ASCII characters please?

I have tried one time, but it seems throwing confusing error. I'd try again in my next PR.

My guess is that is overflows the Uint8Array, as non-ASCII char typically takes more than one byte.

It seems that implementation works in REPL well, but fails in test. My guess is that pseudo-tty test may not support non-ASCII character as input or output? I will fill an issue, if I can confirm that with current API.

REPL

Welcome to Node.js v17.0.0-pre. Type ".help" for more information. > prompt("hello") hello 寿司 '寿司' >

Test

[00:00|% 0|+ 0|- 0]: release test-prompt match failed line=0 expect=^\�\�\�\�\�\�$ actual=寿司 === release test-prompt === Path: pseudo-tty/test-prompt 寿司 what your favorite food? Command: /System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python /Users/qingyudeng/projects/node/test/pseudo-tty/pty_helper.py out/Release/node /Users/qingyudeng/projects/node/test/pseudo-tty/test-prompt.js [00:00|% 100|+ 0|- 1]: Done

Resolved, it is problem with python 2, using python 3 works fine.

Trott · 2021-05-10T06:39:06Z

But my first intention is to facilitate one of the most common tasks.

I think the best way to achieve that for this specific feature would be better documentation, e.g. not bury the await rl.question() example in the last part of https://nodejs.org/api/readline.html#readline_rl_question_query_options_callback, and instead highlight it at the beginning of the readline docs, for example, or generally move the example up in the docs.

Most of our docs would benefit from a longer overview and quick start section at the top that would show typical simple usage, highlighting the best parts of the API. Then the reference section can follow, and it can always always always list things in alphabetical order (or some other order that eliminates human judgment or ambiguity from the ordering).

Ayase-252 · 2021-07-11T12:38:17Z

Closing

globals: introduce prompt on global

fcde071

Fixes: nodejs#37925

github-actions bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels May 5, 2021

Ayase-252 force-pushed the feature/global/introduce-prompt branch from 17c932a to fcde071 Compare May 5, 2021 10:45

aduh95 requested changes May 5, 2021

View reviewed changes

aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 5, 2021

jasnell reviewed May 5, 2021

View reviewed changes

mscdex suggested changes May 5, 2021

View reviewed changes

jasnell reviewed May 5, 2021

View reviewed changes

VoltrexKeyva reviewed May 5, 2021

View reviewed changes

addaleax requested changes May 5, 2021

View reviewed changes

addaleax reviewed May 5, 2021

View reviewed changes

mcollina requested changes May 6, 2021

View reviewed changes

aduh95 reviewed May 7, 2021

View reviewed changes

Ayase-252 closed this Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

globals: introduce `prompt` on global #38552

globals: introduce `prompt` on global #38552

Ayase-252 commented May 5, 2021

nodejs-github-bot commented May 5, 2021

aduh95 left a comment •

edited

Loading

aduh95 May 5, 2021

aduh95 May 5, 2021

aduh95 May 5, 2021

aduh95 May 5, 2021

jasnell commented May 5, 2021

jasnell May 5, 2021

mscdex left a comment •

edited

Loading

jasnell May 5, 2021

jasnell commented May 5, 2021

jasnell May 5, 2021

aduh95 May 5, 2021

Ayase-252 May 6, 2021

VoltrexKeyva May 5, 2021

VoltrexKeyva May 5, 2021

addaleax left a comment

addaleax May 5, 2021

devsnek commented May 5, 2021

jasnell commented May 5, 2021

addaleax commented May 6, 2021

Ayase-252 commented May 6, 2021 •

edited

Loading

addaleax commented May 6, 2021

mcollina left a comment •

edited

Loading

addaleax commented May 6, 2021 •

edited

Loading

Ayase-252 commented May 6, 2021

jasnell commented May 6, 2021

anirudhgiri commented May 6, 2021

aduh95 May 7, 2021

Ayase-252 May 7, 2021

aduh95 May 7, 2021

Ayase-252 May 7, 2021 •

edited

Loading

Trott commented May 10, 2021

Ayase-252 commented Jul 11, 2021

	const LF = '\n'.charCodeAt(0);
	const LF = StringPrototypeCharCodeAt('\n', 0);

	const CR = '\r'.charCodeAt(0);
	const CR = StringPrototypeCharCodeAt('\r', 0);

globals: introduce prompt on global #38552

globals: introduce prompt on global #38552

Conversation

Ayase-252 commented May 5, 2021

Background

API Design Consideration

nodejs-github-bot commented May 5, 2021

aduh95 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 5, 2021

Choose a reason for hiding this comment

mscdex left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented May 5, 2021

jasnell commented May 5, 2021

addaleax commented May 6, 2021

Ayase-252 commented May 6, 2021 • edited Loading

addaleax commented May 6, 2021

mcollina left a comment • edited Loading

Choose a reason for hiding this comment

addaleax commented May 6, 2021 • edited Loading

Ayase-252 commented May 6, 2021

jasnell commented May 6, 2021

anirudhgiri commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ayase-252 May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Trott commented May 10, 2021

Ayase-252 commented Jul 11, 2021

globals: introduce `prompt` on global #38552

globals: introduce `prompt` on global #38552

aduh95 left a comment •

edited

Loading

mscdex left a comment •

edited

Loading

Ayase-252 commented May 6, 2021 •

edited

Loading

mcollina left a comment •

edited

Loading

addaleax commented May 6, 2021 •

edited

Loading

Ayase-252 May 7, 2021 •

edited

Loading