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

Factor out usages of child_process spawning #373

Open
zepumph opened this issue Oct 18, 2024 · 1 comment
Open

Factor out usages of child_process spawning #373

zepumph opened this issue Oct 18, 2024 · 1 comment

Comments

@zepumph
Copy link
Member

zepumph commented Oct 18, 2024

In the last month or two, many more copies of spawn() have been hard coded into our processes. This is because execute is unwieldy and heavy weight, and it too scary to change. At this time we have 8 copies of spawn in chipper and perennial. Let's also look at exec and execSync is we are so inclined.

@samreid
Copy link
Member

samreid commented Nov 8, 2024

In my opinion, execute is problematic and should be avoided:

    // {'reject'|'resolve'} - whether errors should be rejected or resolved.  If errors are resolved, then an object
    //                      - of the form {code:number,stdout:string,stderr:string} is returned. 'resolve' allows usage
    //                      - in Promise.all without exiting on the 1st failure

It sounds like we were unaware of Promise.allSettled.

childProcessShell: cmd !== 'node' && cmd !== 'git' && /^win/.test( process.platform )

This option is never exercised.

      stderr += data;
      grunt.log.debug( `stderr: ${data}` );
      winston.debug( `stderr: ${data}` );

Why do we have 3 channels for the stderr?

Why have an option that radically changes the behavior? Is ExecuteError valuable?

In my opinion, it is clearer and more idiomatic to use spawn directly. We should learn to use it and become comfortable with it. The main/only thing we should factor out to spawn usages is shell: /^win/.test( process.platform )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants