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

globals: introduce prompt on global #38552

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -333,5 +333,6 @@ module.exports = {
btoa: 'readable',
atob: 'readable',
performance: 'readable',
prompt: 'readable'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prompt: 'readable'
prompt: 'readable',

},
};
20 changes: 20 additions & 0 deletions doc/api/globals.md
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,26 @@ added: v0.1.7

The process object. See the [`process` object][] section.

## `prompt([message[, defaultValue]])`
<!-- YAML
added: REPLACEME
-->

<!-- type=global -->

* `message` {string} Optional, Message shown to user. Default: `'prompt'`.
* `defaultValue` {string} Optional, Returned value when user inputs the
empty string.
* Returns: {string|null}

`prompt()` the given message and waits for the user's input.
If the `stdin` is not interactive, it returns `null`.
If default value is not given and the user's input is empty string, it also
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`.
Copy link
Member

Choose a reason for hiding this comment

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

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


## `queueMicrotask(callback)`
<!-- YAML
added: v11.0.0
Expand Down
55 changes: 55 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const {
ObjectSetPrototypeOf,
ReflectGet,
ReflectSet,
Uint8Array,
SymbolToStringTag,
globalThis,
} = primordials;
Expand Down Expand Up @@ -246,6 +247,60 @@ if (!config.noBrowserGlobals) {
return performance;
});

defineOperation(globalThis, 'prompt', function prompt(
message = 'prompt',
defaultValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultValue
defaultValue = null

) {
const { validateString } = require('internal/validators');
validateString(message);
if (defaultValue !== undefined) {
validateString(defaultValue);
}

defaultValue ??= null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultValue ??= null;

if (!process.stdin.isTTY) {
return null;
}

process.stdout.write(message + '\r\n');
const res = readFromStdInSync();
return res || defaultValue;

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

Choose a reason for hiding this comment

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

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

Use primordials for consistency.

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

Choose a reason for hiding this comment

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

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

Same case as above.

const charaBuf = new Uint8Array(1);
const stdfd = process.platform === 'win32' ?
process.stdin.fd :
fs.openSync('/dev/tty', 'r');
const res = [];

while (true) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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');

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

if (charaBuf[0] === LF) {
break;
}
res.push(CR);
if (bytesRead === 0) {
break;
}
}
if (charaBuf[0] === LF) {
break;
}
res.push(charaBuf[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
res.push(charaBuf[0]);
ArrayPrototypePush(res, charaBuf[0]);

}
return new TextDecoder().decode(new Uint8Array(res));
}
});

// Non-standard extensions:
defineOperation(globalThis, 'clearImmediate', timers.clearImmediate);
defineOperation(globalThis, 'setImmediate', timers.setImmediate);
Expand Down
4 changes: 4 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,10 @@ if (global.performance) {
knownGlobals.push(global.performance);
}

if (global.prompt) {
knownGlobals.push(global.prompt);
}

function allowGlobals(...allowlist) {
knownGlobals = knownGlobals.concat(allowlist);
}
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-global.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ builtinModules.forEach((moduleName) => {
'clearInterval',
'clearTimeout',
'performance',
'prompt',
'setImmediate',
'setInterval',
'setTimeout',
Expand Down
2 changes: 2 additions & 0 deletions test/pseudo-tty/test-prompt.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
sushi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test with non-ASCII characters please?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Ayase-252 Ayase-252 May 7, 2021

Choose a reason for hiding this comment

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

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.


12 changes: 12 additions & 0 deletions test/pseudo-tty/test-prompt.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';

const common = require('../common');
const assert = require('assert');

(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

const resp1 = prompt('what your favorite food?');
assert.strictEqual(resp1, 'sushi');

const resp2 = prompt('what is the default value?', 'DEFAULT');
assert.strictEqual(resp2, 'DEFAULT');
})().then(common.mustCall(() => {}));
3 changes: 3 additions & 0 deletions test/pseudo-tty/test-prompt.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
sushi
what your favorite food?
what is the default value?