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

shell option for execFile and execFileSync in child_process module not documented #18199

Closed
mojavelinux opened this issue Jan 17, 2018 · 3 comments
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@mojavelinux
Copy link

  • Version: 8.9.4
  • Platform: n/a
  • Subsystem: child_process

The shell option for the execFile and execFileSync functions in the child_process module is not documented. However, it clearly works, as this mocha/chai test proves:

const { execFile, execFileSync } = require('child_process')
const expect = require('chai').expect

const EXEC_OPTS = { encoding: 'utf8', shell: process.env.SHELL }

describe('execFile()', () => {
  it('should run command using specified shell', async () => {
    let fn
    try {
      const result = await new Promise((resolve, reject) => {
        execFile('ls', ['*'], EXEC_OPTS, (err, stdout) =>
          err ? reject(err) : resolve(stdout))
      })
      fn = () => result
    } catch (err) {
      fn = () => { throw err }
    }
    expect(fn).to.not.throw()
  })
})

describe('execFileSync()', () => {
  it('should run command using specified shell', () => {
    expect(() => execFileSync('ls', ['*'], EXEC_OPTS)).to.not.throw()
  })
})

I'm happy to add documentation for this option. I just need to know if this behavior is intention or whether it's the result of leaky internals.

@bnoordhuis bnoordhuis added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jan 17, 2018
@bnoordhuis
Copy link
Member

I suppose it's undocumented because of this paragraph:

The child_process.execFile() function is similar to child_process.exec() except that it does not spawn a shell. Rather, the specified executable file is spawned directly as a new process making it slightly more efficient than child_process.exec().

@mojavelinux
Copy link
Author

I get that's the default behavior. But this function is still useful for executing in a shell because it accepts arguments as an array instead of a string as exec does. Depending on the circumstances, that can prove to be useful if the arguments are already prepared.

@bnoordhuis bnoordhuis added the good first issue Issues that are suitable for first-time contributors. label Jan 18, 2018
@jvelezpo
Copy link
Contributor

I can work on it, and it will be my first issue and my first contribution to the core 😎

MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: #18237
Fixes: #18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: #18237
Fixes: #18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: #18237
Fixes: #18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: #18237
Fixes: #18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Mar 28, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: #18237
Fixes: #18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Mar 30, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: #18237
Fixes: #18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

PR-URL: nodejs#18237
Fixes: nodejs#18199
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

3 participants