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

Add sync command api #2

Closed
sigmaSd opened this issue Jul 13, 2022 · 3 comments
Closed

Add sync command api #2

sigmaSd opened this issue Jul 13, 2022 · 3 comments
Labels
wontfix This will not be worked on

Comments

@sigmaSd
Copy link
Contributor

sigmaSd commented Jul 13, 2022

Hello, thanks for this project

I mostly use this in repl so I end up writing a lot of await $c1 await $c2, I feel like those awaits are really unneeded and verbose. It would be nice if ax supported a sync api using Deno.spawnSync

@sigmaSd sigmaSd changed the title Allow sync command Add sync command api Jul 13, 2022
@dsherret dsherret added the enhancement New feature or request label Jul 14, 2022
@dsherret
Copy link
Owner

I've been thinking about this and it wouldn't be much better because instead of adding await on the front you would have to remember to type .spawnSync() at the end or something like that to kick it off. Additionally, it blocking synchronously would prevent some scenarios from being possible... the library would need to throw if the user did certain things.

So, overall I don't think this should be done; however, I thought of what I think is a better alternative. See here: denoland/deno#15209

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Jul 15, 2022

I don't get the typing of spawnSync part, the user would just type $'ls' and it would internally use it. (for example that's what I do here https://github.com/sigmaSd/ds)

Additionally, it blocking synchronously would prevent some scenarios from being possible... the library would need to throw if the user did certain things.

I propose to not replace the current api with this but to add it as an alternative api

I guess denoland/deno#15209 might work as well but personally I'm not fan of adding repl specific behavior, so the code you write in the repl you can't just copy paste it into a file (you'll need to add back those await)

@dsherret dsherret added wontfix This will not be worked on and removed enhancement New feature or request labels Jul 15, 2022
@dsherret
Copy link
Owner

dsherret commented Jul 15, 2022

There's builder steps per command, so it would probably need to be something like:

$`echo $MY_VAR`.env("MY_VAR", "test").spawnSync();
// or
$.env("MY_VAR", "test").spawnSync(`echo $MY_VAR`);

So this would be just as verbose as adding await.

I guess denoland/deno#15209 might work as well but personally I'm not fan of adding repl specific behavior, so the code you write in the repl you can't just copy paste it into a file (you'll need to add back those await)

I don't think this is that big of a deal because if someone wants to be able to copy and paste then they can add await keywords.

Anyway, I'm marking this as something that won't be done because adapting this API would be just as verbose and a synchronous API leads to limitations where things like only synchronous writers can be provided to pipe or a potential deadlock could occur if a process is waiting on stdin via a writer that executes in JavaScript (so the library would need to throw in this scenario... just leads to a lot of code to maintain for not much benefit with way more considerations).

Edit: Actually the more I think about it the more I agree with your statement. It creates a bad habit of not providing an await keyword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants