Fix documentation of process.argv[0] by tarungarg546 · Pull Request #7449 · nodejs/node

tarungarg546 · 2016-06-27T20:19:48Z

Checklist

documentation is changed or added

Affected core subsystem(s)

doc

Description of change

This PR is in reference for this issue.

Modified docs to reflect original value of process.argv[0]

cjihrig · 2016-06-27T20:21:40Z

Could you format the commit message according to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit.

The current documentation states that if run something like node app.js then in our process.argv array first elements is node, but actually its process.execPath not node as documentation currently suggests This commit fixes this documentation bug. Fixes : nodejs#7434 PR-URL: nodejs#7449

mscdex · 2016-06-27T21:21:09Z

 The `process.argv` property returns a array containing the command line
 arguments passed when the Node.js process was launched. The first element will
-be 'node', the second element will be the name of the JavaScript file. The
+be  [`process.execPath()`], the second element will be the name of the JavaScript file. The


While we're in here, we may as well change "name of the JavaScript file" to something like "path to the JavaScript file."

Seems fair enough?
What say @cjihrig

I agree with @mscdex. Also, this line should be wrapped at 80 characters.

Hm. Is it a bug then that running a file named test.js using node test will lead to process.argv[1] === '/path/to/test', not /path/to/test.js?

hmm.. that's an interesting question... but I think that's ok. While there is the potential for conflicts because of the missing .js, it's not likely to have a serious impact. I guess that someone could come along and create a separate file named test after the app was started that would lead to issues but that seems like a relatively safe edge case.

Done @cjihrig

With this commit everyline of process documentation is wrapped in 80 characters and there are some changes for documention of process.arv[0] and process.argv[1]

cjihrig · 2016-06-29T16:21:47Z

 arguments passed when the Node.js process was launched. The first element will
-be 'node', the second element will be the name of the JavaScript file. The
-remaining elements will be any additional command line arguments.
+be [`process.execPath()`], second element will be the path to JavaScript file.


I think the part about the second element should be a separate sentence:
The second element will be the path to the JavaScript file being executed.

Done
I think the part about the second element should be a separate sentence:

It looks like it's still one sentence, just with 'the' removed in two places?

Oh sorry forgot to push changes.

With this commit everyline of process documentation is wrapped in 80 characters and there are some changes for documention of process.arv[0] and process.argv[1]

cjihrig · 2016-06-29T17:23:46Z

-be 'node', the second element will be the name of the JavaScript file. The
-remaining elements will be any additional command line arguments.
+arguments passed when the Node.js process was launched. 
+The first element will be [`process.execPath()`],


No need to split all of these across lines. Just wrap them at 80 characters. Also, the sentence should end with a period, not a comma.

With this commit everyline of process documentation is wrapped in 80 characters and there are some changes for documention of process.arv[0] and process.argv[1]

cjihrig · 2016-06-29T17:46:07Z

-arguments passed when the Node.js process was launched. The first element will
-be 'node', the second element will be the name of the JavaScript file. The
-remaining elements will be any additional command line arguments.
+The `process.argv` property returns a array containing the command line 


These changes are just some grammatical changes like using `an` not `a` and space after period etc.

cjihrig · 2016-06-29T20:06:50Z

LGTM

mscdex · 2016-06-29T20:12:22Z

LGTM

Dmitry-Me · 2016-07-01T08:05:47Z

So sad... we'll have to change our code...

addaleax · 2016-07-01T08:08:14Z

@Dmitry-Me It might help if you could describe your use case (but probably better in the original issue, as long as it doesn’t concern this documentation change itself).

Dmitry-Me · 2016-07-01T08:25:11Z

@addaleax We host a piece of sample code for our users. Because it's JavaScript some of them try to run it inside web browser, which of course doesn't work. So we need a check that effectively "code is being run under NodeJS". Earlier we could just check that argv[0]=='node' but now we'll have to extract the executable name, perhaps remove the extension (somepath/node.exe on Windows and I guess it's just somepath/node on Linux) and all that stuff.

addaleax · 2016-07-01T08:33:58Z

@Dmitry-Me Checking argv[0] == 'node' wouldn’t be very reliable anyway, e.g. it wouldn’t stop anyone from giving using the full node path as the first argument. If you really need some kind of check, you could test for e.g. the presence of the global process object.

Dmitry-Me · 2016-07-01T08:38:34Z

@addaleax Would just checking for presence of process be enough?

addaleax · 2016-07-01T08:43:33Z

@Dmitry-Me For the scenario you described, i.e. users trying to run the file in a browser with nothing else going on, yes.

Google will gladly lead you to a thousand different ways to check whether code is being run under node or not (including testing process, module or require).

EDIT: Again, if there are more follow-up questions, these probably are better asked at the original issue (nodejs/help might be a better place, too).

addaleax · 2016-07-01T08:44:44Z

-remaining elements will be any additional command line arguments.
+The `process.argv` property returns an array containing the command line 
+arguments passed when the Node.js process was launched. The first element will 
+be [`process.execPath()`]. The second element will be the path to the 


process.execPath is not a function, so I’d drop the parentheses?

addaleax · 2016-07-01T08:44:52Z

LGTM with a nit.

jasnell · 2016-07-01T16:56:24Z

LGTM

tarungarg546 · 2016-07-02T19:18:58Z

What's next?

addaleax · 2016-07-04T14:51:15Z

Landed in 475dc43, thank for the contribution!

The current documentation states that if run something like `node app.js` then in our process.argv array first elements is `node`, but actually it's `process.execPath` not `node` as documentation currently suggests. Fixes: #7434 PR-URL: #7449 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

For historical and other reasons, node overwrites `argv[0]` on startup. See - 2c6f79c, - nodejs#7434 - nodejs#7449 - nodejs#7696 For cases where it may be useful, save the original value of `argv[0]` in `process.argv0`

For historical and other reasons, node overwrites `argv[0]` on startup. See - 2c6f79c, - #7434 - #7449 - #7696 For cases where it may be useful, save the original value of `argv[0]` in `process.argv0` PR-URL: #7696 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Updated ReadMe fornodejs#7434
7e8ab82

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Jun 27, 2016

tarungarg546 mentioned this pull request Jun 27, 2016
process.argv[0] contains full node.exe path on Windows, docs say it must be just "node" #7434
Closed

mscdex reviewed Jun 27, 2016
View reviewed changes

Fishrock123 changed the title ~~Updated ReadMe for #7434~~Fix documentation of process.argv[0]Jun 28, 2016

Changes as suggested by@mscdex&@cjihrig
e620fa0
With this commit everyline of process documentation is wrapped in 80 characters and there are some changes for documention of process.arv[0] and process.argv[1]

cjihrig reviewed Jun 29, 2016
View reviewed changes

Changes as suggested by@mscdex&@cjihrig
7b0096e
With this commit everyline of process documentation is wrapped in 80 characters and there are some changes for documention of process.arv[0] and process.argv[1]

cjihrig reviewed Jun 29, 2016
View reviewed changes

Changes as suggested by@mscdex&@cjihrig
4cd4026
With this commit everyline of process documentation is wrapped in 80 characters and there are some changes for documention of process.arv[0] and process.argv[1]

cjihrig reviewed Jun 29, 2016
View reviewed changes

Minor grammatical changes in doc for process api
9afa7fd
These changes are just some grammatical changes like using `an` not `a` and space after period etc.

addaleax reviewed Jul 1, 2016
View reviewed changes

Drop paranthesis around process.execPath
69456e4

addaleax closed this Jul 4, 2016

Fishrock123 mentioned this pull request Jul 5, 2016
Propose v6.3.0 (v2) #7550
Merged

MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed

MylesBorins added the dont-land-on-v4.x label Jul 12, 2016

addaleax mentioned this pull request Jul 13, 2016
lib: allow custom names for spawned processes #7696
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

tarungarg546 commented Jun 27, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrig commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jun 29, 2016

Uh oh!

mscdex commented Jun 29, 2016

Uh oh!

Dmitry-Me commented Jul 1, 2016

Uh oh!

addaleax commented Jul 1, 2016

Uh oh!

Dmitry-Me commented Jul 1, 2016

Uh oh!

addaleax commented Jul 1, 2016

Uh oh!

Dmitry-Me commented Jul 1, 2016

Uh oh!

addaleax commented Jul 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 1, 2016

Uh oh!

jasnell commented Jul 1, 2016

Uh oh!

tarungarg546 commented Jul 2, 2016

Uh oh!

addaleax commented Jul 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented Jul 1, 2016•
edited
Loading