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.

Sign up for GitHub

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

Jump to bottom

Expose execa Options for Configuring Its Child Process Spawning Behavior #1019

Closed
ZheyangSong opened this issue Sep 24, 2021 · 4 comments · Fixed by #1387
Closed

Expose execa Options for Configuring Its Child Process Spawning Behavior #1019

ZheyangSong opened this issue Sep 24, 2021 · 4 comments · Fixed by #1387

Comments

@ZheyangSong
Copy link

ZheyangSong commented Sep 24, 2021 •
edited
Loading

Description

I try to use lint-staged to run a custom script. The script reads input from different source based on process.stdin.isTTY.

The code structure is roughly like this:

// custom-script.js
...
const inputStream = process.stdin.isTTY ? Readable.from(process.argv.slice(2)) : process.stdin;

inputStream.on('data', ...);
...

However, when this script is invoked by lint-staged with configured command node custome-script.js, it wrongfully expects input from process.stdin while the input is from process.argv. This is due to how nodeJS's child_process module works: https://nodejs.org/api/child_process.html#child_process_options_stdio

The default pipe stdio option marks process.stdin.isTTY false in spawned child process. Hence, my script gets confused and waits for input while letting lint-staged hang forever...

I'd like to be able to adjust the child_process.spawn's behavior via lint-staged's configuration somehow.

Related code segment: https://github.com/okonet/lint-staged/blob/0ef25e81a150ae59749d28565b305c97ec932baa/lib/resolveTaskFn.js#L92-L105

Steps to reproduce

Try to invoke below script with lint-staged:

const { once } = require('events');
const { Readable } = require('stream');

const inputStream = process.stdin.isTTY ? Readable.from(process.argv.slice(2)) : process.stdin;

(async () => {
  inputStream.on('data', console.log);
  await once(inputStream, 'end');
}())

Debug Logs

N/A

Environment

  • OS: macOS Big Sur
  • Node.js: v12.13.1
  • lint-staged: 11.1.2
@ZheyangSong ZheyangSong mentioned this issue Sep 24, 2021
Closed
@iiroj
Copy link
Member

iiroj commented Sep 27, 2021

Would it make sense to expose an execaOptions key in the Node.js API? This would make it an advanced configuration option, and keep the cli simple.

https://github.com/okonet/lint-staged#can-i-use-lint-staged-via-node

@ZheyangSong
Copy link
Author

ZheyangSong commented Sep 27, 2021 •
edited
Loading

@iiroj it would in general. By looking at the code, I wonder if the author wanted to have some behavior control on the execa (maybe to support the basic function of lint-staged). Otherwise, giving users full control on execa is the most straight-forward approach. :)

@iiroj
Copy link
Member

iiroj commented Sep 27, 2021

Hello,

the current execa options are hard-coded as follows:

  • preferLocal: true so that you can more easily run binaries installed into the current project's node_modules/
  • reject: false so that the child doesn't throw and lint-staged can more easily print its stderr
  • shell passed directly as an option, to enable built-in shell behavior
  • possibly cwd = gitDir, when the executed command is git, so that those are always run in the repo root (instead of possible subdirectory).

I can see the last option being problematic when overidden, but having a new execaOptions object spread over the defaults would IMO be easy, powerful, and advanced, so I guess leaving it unadvertised would be preferable.

@ZheyangSong
Copy link
Author

ZheyangSong commented Sep 28, 2021 •
edited
Loading

@iiroj Interesting. With the consideration of cwd, I personally prefer to selectively exclude cwd from the configurable execOptions. It's a good trade-off the tool makes for users to prevent unexpected behaviors. I guess the needs to change cwd should be rare, given how commands are invoked by lint-staged.

Alternatively, if the change will allow configuring all execa options, I think one detailed debug message that highlights cwd change (if overridden) should also help a lot when weird things happen.

I'm not sure about the idea of unadvertised APIs. Personally, I'm not a fan of it. I usually find it discouraging and disappointing if some undocumented API is discovered after going through a whole bunch of code...Such API usually makes me hesitate to use it as well. :)

@chdsbd chdsbd mentioned this issue Apr 12, 2023
Closed
@mtgto mtgto mentioned this issue May 2, 2023
Closed
@iiroj iiroj mentioned this issue Jan 24, 2024
Merged
@iiroj iiroj linked a pull request Jan 24, 2024 that will close this issue
fix: ignore stdin of spawned commands #1387
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: ignore stdin of spawned commands lint-staged/lint-staged
2 participants
@ZheyangSong @iiroj