lib: improve option shell on child_process

[himself65]

fix #27120

shell add Array option to decode parameters

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ZYSzys]

This should need some tests.

[himself65]

yes, i will

but i’m thinking if this solution is the good way

[BridgeAR]

@nodejs/child_process do we maybe want to make this OS agnostic and rename this to shellParameters? That way we'd have a unified way to pass flags to the shell.

[refack]

@nodejs/child_process do we maybe want to make this OS agnostic and rename this to shellParameters? That way we'd have a unified way to pass flags to the shell.

I was thinking about an even more generic approach; make shell take an array value so that

1. exec('"/path/to/test file/test.sh" arg1 arg2');

   Uses default shell with default cli arg

2. exec('"/path/to/test file/test.sh" arg1 arg2', { shell: 'bash' });

   Uses specified shell with default cli arg

3. exec('"/path/to/test file/test.sh" arg1 arg2',  { shell: ['bash', '-xe', '-x'] });

   Uses specified shell with specified cli arg

[refack]

P.S. @himself65, thank you very much for taking this on 🎩

P.P.S. This will need a new test case. IMHO it could fit somewhere above this condition

node/test/parallel/test-child-process-exec-any-shells-windows.js

Lines 8 to 10 in 528d100

	// This test is only relevant on Windows.
	if (!common.isWindows)
	common.skip('Windows specific test.');

[BridgeAR]

@refack I like the array notation. I think that's even a better approach than adding a new option.

[himself65]

I try

[himself65]

@BridgeAR @refack

I have done the solution of array notation. Any reviews?

[BridgeAR]

We have to document the new behavior properly (this is especially important on Windows). Other than that it's LGTM % nits.

[BridgeAR]

The code itself LGTM. We just have to update the documentation to include the information and also add an changes entry to the docs that highlights when this feature was added.

[himself65]

hmmm, let me update the documentation? but I am not good at this.

[gireeshpunathil]

@himself65 - this needs a rebase now

[himself65]

rebased

[aduh95]

@himself65 this needs a rebase.

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

[Taitava]

Could this be checked again? 🙂 I don't have the technical knowledge regarding developing Node.js, so I don't know what actually needs to be done, but I would be interested in the ability to define shell parameters.

Thanks! 🌞