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

cmd: support dash as stdin alias #13012

Closed
wants to merge 1 commit into from
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion doc/api/cli.md
Expand Up @@ -10,7 +10,7 @@ To view this documentation as a manual page in a terminal, run `man node`.

## Synopsis

`node [options] [v8 options] [script.js | -e "script"] [--] [arguments]`
`node [options] [v8 options] [script.js | -e "script" | -] [--] [arguments]`
Copy link
Contributor Author

@ebraminio ebraminio May 20, 2017

Choose a reason for hiding this comment

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

Removed space after the dash as its section start also is not space padded.


`node debug [script.js | -e "script" | <host>:<port>] …`

Expand Down Expand Up @@ -345,6 +345,17 @@ added: v0.11.15

Specify ICU data load path. (overrides `NODE_ICU_DATA`)


### `-`
<!-- YAML
added: REPLACEME
-->

Alias for stdin, analogous to the use of - in other command line utilities,
meaning that the script will be read from stdin, and the rest of the options
are passed to that script.


### `--`
<!-- YAML
added: v7.5.0
Expand Down
2 changes: 1 addition & 1 deletion doc/api/synopsis.md
Expand Up @@ -2,7 +2,7 @@

<!--type=misc-->

`node [options] [v8 options] [script.js | -e "script"] [arguments]`
`node [options] [v8 options] [script.js | -e "script" | - ] [arguments]`

Please see the [Command Line Options][] document for information about
different options and ways to run scripts with Node.js.
Expand Down
11 changes: 10 additions & 1 deletion doc/node.1
Expand Up @@ -36,7 +36,10 @@ node \- Server-side JavaScript runtime
.RI [ v8\ options ]
.RI [ script.js \ |
.B -e
.RI \&" script \&"]
.RI \&" script \&"
.R |
.B -
.R ]
Copy link
Member

Choose a reason for hiding this comment

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

This can be condensed into one line as .RB \| - ] I think. I forget how exactly the escapes work for the | character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like .RB |\ -\ ] worked but no bolding of dash, is that OK?

Copy link
Contributor Author

@ebraminio ebraminio May 17, 2017

Choose a reason for hiding this comment

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

Never had experience of doc writing using troff before, will apply whatever you conclude of course.

Copy link
Member

Choose a reason for hiding this comment

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

Just took a look and this does appear to be correct.

.B [--]
.RI [ arguments ]
.br
Expand Down Expand Up @@ -225,6 +228,12 @@ See \fBSSL_CERT_DIR\fR and \fBSSL_CERT_FILE\fR.
.BR \-\-icu\-data\-dir =\fIfile\fR
Specify ICU data load path. (overrides \fBNODE_ICU_DATA\fR)

.TP
.BR \-\fR
Alias for stdin, analogous to the use of - in other command line utilities,
meaning that the script will be read from stdin, and the rest of the options
are passed to that script.

.TP
.BR \-\-\fR
Indicate the end of node options. Pass the rest of the arguments to the script.
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/bootstrap_node.js
Expand Up @@ -123,7 +123,7 @@
const internalModule = NativeModule.require('internal/module');
internalModule.addBuiltinLibsToObject(global);
evalScript('[eval]');
} else if (process.argv[1]) {
} else if (process.argv[1] && process.argv[1] !== '-') {
Copy link
Contributor

@refack refack May 13, 2017

Choose a reason for hiding this comment

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

IMHO -- is much more common alias for "the rest will be piped through stdin". If it's not too much work I'd be happy if you adjust.
So you could do echo "console.log(process.argv[2])" | node -h --

Copy link
Contributor

Choose a reason for hiding this comment

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

Got you -h is an arg to the script. Not to node.
Retracting.

Copy link
Member

Choose a reason for hiding this comment

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

We already use -- for separating arguments, and that’s what it’s commonly used for (on Unixes, at least). - is the most common choice for “stdin”, by far.

// make process.argv[1] into a full path
const path = NativeModule.require('path');
process.argv[1] = path.resolve(process.argv[1]);
Expand Down
6 changes: 5 additions & 1 deletion src/node.cc
Expand Up @@ -3586,7 +3586,7 @@ void LoadEnvironment(Environment* env) {
static void PrintHelp() {
// XXX: If you add an option here, please also add it to doc/node.1 and
// doc/api/cli.md
printf("Usage: node [options] [ -e script | script.js ] [arguments]\n"
printf("Usage: node [options] [ -e script | script.js | - ] [arguments]\n"
" node inspect script.js [arguments]\n"
"\n"
"Options:\n"
Expand All @@ -3598,6 +3598,8 @@ static void PrintHelp() {
" does not appear to be a terminal\n"
" -r, --require module to preload (option can be "
"repeated)\n"
" - script read from stdin (default; "
"interactive mode if a tty)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added this line from the python -h, please review it before the merge.

Copy link
Contributor

@refack refack May 20, 2017

Choose a reason for hiding this comment

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

You mean node -h? ahh "from"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant I've copied this literally from the similar place on python -h :)

#if HAVE_INSPECTOR
" --inspect[=[host:]port] activate inspector on host:port\n"
" (default: 127.0.0.1:9229)\n"
Expand Down Expand Up @@ -3903,6 +3905,8 @@ static void ParseArgs(int* argc,
} else if (strcmp(arg, "--expose-internals") == 0 ||
strcmp(arg, "--expose_internals") == 0) {
config_expose_internals = true;
} else if (strcmp(arg, "-") == 0) {
break;
} else if (strcmp(arg, "--") == 0) {
index += 1;
break;
Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-stdin-script-child-option.js
@@ -0,0 +1,17 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const expected = '--option-to-be-seen-on-child';

const { spawn } = require('child_process');
const child = spawn(process.execPath, ['-', expected], { stdio: 'pipe' });

child.stdin.end('console.log(process.argv[2])');

let actual = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (chunk) => actual += chunk);
child.stdout.on('end', common.mustCall(() => {
assert.strictEqual(actual.trim(), expected);
}));
49 changes: 25 additions & 24 deletions test/parallel/test-stdin-script-child.js
Expand Up @@ -2,31 +2,32 @@
const common = require('../common');
const assert = require('assert');

const spawn = require('child_process').spawn;
const child = spawn(process.execPath, [], {
env: Object.assign(process.env, {
NODE_DEBUG: process.argv[2]
})
});
const wanted = `${child.pid}\n`;
let found = '';
const { spawn } = require('child_process');
for (const args of [[], ['-']]) {
const child = spawn(process.execPath, args, {
env: Object.assign(process.env, {
NODE_DEBUG: process.argv[2]
})
});
const wanted = `${child.pid}\n`;
let found = '';

child.stdout.setEncoding('utf8');
child.stdout.on('data', function(c) {
found += c;
});
child.stdout.setEncoding('utf8');
child.stdout.on('data', function(c) {
found += c;
});

child.stderr.setEncoding('utf8');
child.stderr.on('data', function(c) {
console.error(`> ${c.trim().split(/\n/).join('\n> ')}`);
});
child.stderr.setEncoding('utf8');
child.stderr.on('data', function(c) {
console.error(`> ${c.trim().split(/\n/).join('\n> ')}`);
});

child.on('close', common.mustCall(function(c) {
assert.strictEqual(c, 0);
assert.strictEqual(found, wanted);
console.log('ok');
}));
child.on('close', common.mustCall(function(c) {
assert.strictEqual(c, 0);
assert.strictEqual(found, wanted);
}));

setTimeout(function() {
child.stdin.end('console.log(process.pid)');
}, 1);
setTimeout(function() {
child.stdin.end('console.log(process.pid)');
}, 1);
}
Copy link
Contributor

@refack refack May 13, 2017

Choose a reason for hiding this comment

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

Nit: We like an \n on the last line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for both