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

fix: child process cmd: option except iterable array #429

Merged
merged 3 commits into from
Nov 23, 2024

Conversation

SRWieZ
Copy link
Contributor

@SRWieZ SRWieZ commented Nov 21, 2024

I encountered an error that was not understandable at first glance.
TypeScript was complaining that cmd: was not iterable.

This error was crashing the app before even showing a window.

For context, I was writing this

ChildProcess::artisan(cmd: ['app:victron-mqtt','--vrmid' => 'myvrmid'], alias: 'something');

instead of

ChildProcess::artisan(cmd: ['app:victron-mqtt','--vrmid=myvrmid'], alias: 'something');

Did you notice the difference?


Screenshot of the error:
Capture d’écran 2024-11-21 à 23 49 58

TypeError: settings.cmd is not iterable at startPhpProcess
@SRWieZ
Copy link
Contributor Author

SRWieZ commented Nov 21, 2024

Oh, and I apologise for the branch names.

I make quick fixes by directly modifying files in my vendor directory and using the web version of GitHub for the PR 🙈

I find this method faster when it only involves one file in one repo.

This allows me to report and fix bugs while staying focused on my apps 😄

Copy link
Member

@simonhamp simonhamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! I think what you've bumped into is that as a Laravel/PHP dev, our expectation is that a command's parameters are a keyed array, which I think is actually a good interface and pretty common across other PHP libraries that do something similar.

But Electron's utilityProcess expects a flat JS array of strings (i.e. a list).

I'm not sure simply wrapping in array_values is enough here... devs may find the fact that they can't use named array items unusual and even confusing/frustrating that some of their command definition is completely ignored.

I think we could do one of a couple of things, either:

  • Accept the args as a hash table (assoc. array) and do the work to flatten the keys onto their values before sending to Electron, so that...
    ['app:some-command', '--arg1' => 'val', '--arg2' => 'val']
    // Becomes
    ['app:some-command', '--arg1=val', '--arg2=val']
  • Add an exception when we see an associative array and update the docs to explicitly state that the command must be a flat array such that:
    ['app:some-command', '--arg1' => 'val', '--arg2' => 'val'] // => throws new ChildProcessException
    ['app:some-command', '--arg1=val', '--arg2=val'] // succeeds

I honestly don't mind whichever of these approaches we go for

@gwleuverink
Copy link
Contributor

gwleuverink commented Nov 22, 2024

I'm down to support this associative syntax. I touched it when adding mixed string|array arguments.

Laravel's process facade doesn't support it (and doesn't throw an exception)
So an argument for syntax parity could be made as wel

$result = Illuminate\Process::run(['app:some-command', '--arg1' => 'val', '--arg2' => 'val']);

$result->command(); // "'app:some-command' 'val' 'val'"

@gwleuverink
Copy link
Contributor

Another note. Now I've thought about it a bit more.

Some commands don't adhere to this syntax (using an = sign). As I understand it this is up to the programmer writing the command.

So both of these could be valid depending on the program: --arg=param or --arg param

Taking that into consideration I'm leaning towards sticking to only flat arrays & add the Exception for good measure.

@SRWieZ
Copy link
Contributor Author

SRWieZ commented Nov 22, 2024

@simonhamp: Interesting! I think what you've bumped into is that as a Laravel/PHP dev, our expectation is that a command's parameters are a keyed array, which I think is actually a good interface and pretty common across other PHP libraries that do something similar.

I'm not sure, I may have invented it.
Neither Symphony nor Laravel supports this, and I have never used any others.

@simonhamp: I honestly don't mind whichever of these approaches we go for

Something I agree with you, this current implement feels like a soft fail.

@gwleuverink: Taking that into consideration I'm leaning towards sticking to only flat arrays & add the Exception for good measure.

Me too. I'm gonna implement it right now.

@simonhamp
Copy link
Member

simonhamp commented Nov 23, 2024

Neither Symphony nor Laravel supports this, and I have never used any others.

No way. I could've sworn I've also done this in Laravel... my LLM cells are hallucinating

On this basis, I'd shoot for whatever ends up being the most consistent with current behaviour elsewhere - if that is just array_values, then that's fine

@simonhamp simonhamp merged commit 98405ae into NativePHP:main Nov 23, 2024
21 checks passed
@SRWieZ
Copy link
Contributor Author

SRWieZ commented Nov 23, 2024

I propose a middle ground.

  1. Instead of checking at runtime by comparing array_keys to a range (e07c964), kind of weird.
  2. I suggest using array_values and adding a hint for static analyzers and IDEs to indicate that an indexed array of strings is expected (539e875)

Vote, one or two. I vote 2.

@SRWieZ
Copy link
Contributor Author

SRWieZ commented Nov 23, 2024

Oh! vote ended, it's merged 🤣

@simonhamp
Copy link
Member

Sorry, I thought it was done already... might be best to move back into draft if you're still rocking on it

@SRWieZ
Copy link
Contributor Author

SRWieZ commented Nov 23, 2024

Sorry, I thought it was done already... might be best to move back into draft if you're still rocking on it

My bad.

On this basis, I'd shoot for whatever ends up being the most consistent with current behaviour elsewhere - if that is just array_values, then that's fine

At the last minute, I thought I might have a better solution that aligns with other libraries and helps the developer avoid mistakes.

I hope you're okay with the final change.

@simonhamp
Copy link
Member

I hope you're okay with the final change.

yep, that's why I merged 😄

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

Successfully merging this pull request may close these issues.

3 participants