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

feature request: shorter $.sync #581

Closed
Zamiell opened this issue Oct 28, 2023 · 24 comments · Fixed by #594
Closed

feature request: shorter $.sync #581

Zamiell opened this issue Oct 28, 2023 · 24 comments · Fixed by #594

Comments

@Zamiell
Copy link

Zamiell commented Oct 28, 2023

Hello, and thanks for the library.

I love the $.sync command, but it is too long. I want to make scripts that are nice and short.

e.g.

// Bad
await $`foo`;
await $`bar`;
await $`baz`;

// Also bad
$.sync`foo`;
$.sync`bar`;
$.sync`baz`;

I propose a default alias for $.sync as s.

// Good!
s`foo`;
s`bar`;
s`baz`;

I know that I can do this for my own projects by wrapping $.sync and exporting it, but I think this feature should be for all users!

@sindresorhus
Copy link
Owner

const s = $.sync;

// Good!
s`foo`;
s`bar`;
s`baz`;

@Zamiell
Copy link
Author

Zamiell commented Oct 28, 2023

Hi @sindresorhus, thanks for the quick reply.

As I said in the original post, I know that I can do this for my own projects by wrapping $.sync exactly like you showed in your reply. But I don't think that everyone should have to do this - it adds a lot of boilerplate to have to do this in every single script! So, I think this feature should be usable by default for everybody.

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 28, 2023

Hi @Zamiell,

I agree with @sindresorhus that, since const s = $.sync is quite simple and straightforward, there might not be a need for us to export a shorter alias for $.sync.

@Zamiell
Copy link
Author

Zamiell commented Oct 28, 2023

@ehmicky Thanks for the quick reply. I'm sad to hear that you don't like the idea, as I think having s as an export would be really nice for users. Maybe I can change your mind? Consider the following:

  1. The reason that execa arbitrarily chooses $ over something like runCommand is because of how nice and short it is. I think an important part of why this library is so useful and popular is that $ is only a single character! So, by having the synchronous version of the command also as a single character, it could really improve the usability.

  2. Having to type out const s = $.sync; over and over in every single script that you write is really tedious! It adds a lot of boilerplate for what should otherwise be a really nice API.

  3. Having an official export for s gives you the ability to auto import in VSCode. To illustrate why this is important, let's imagine that I want to start up a new script, so I start typing like this:

image

I get a red squiggly line underneath. So now, I have to go manually open up one of my other scripts, find the line that says const s = $.sync;, copy it to clipboard, go back to my original file, paste it, and then now I get a red squiggly line over $, so I have to auto import that. So now my final script looks like:

import { $ } from "execa";

const s = $.sync;

s`npx tsx`;
s`npx eslint .`;

Overall, this is not a great user experience. But if s were officially exported from execa, then all I would have to do is put the cursor over s, auto-import, and I would be done:

import { s } from "execa";

s`npx tsx`;
s`npx eslint .`;

Much easier, and no manual copy-pasting involved! Because auto-importing is doing all of the work.

@Zamiell
Copy link
Author

Zamiell commented Oct 28, 2023

By the way, I think another great feature would be o for "get the command output synchronously".

Consider the following code:

import { $ } from "execa";

const branch = $.sync`git branch --show-current`.stdout;
if (branch === "main") {
  // Do something.
}

This is kind of ugly. I think it's a really common pattern to run a command and then put the output into a variable. So that pattern could be greatly simplified by execa like this:

import { o } from "execa";

const branch = o`git branch --show-current`;
if (branch === "main") {
  // Do something.
}

What do you think?

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 28, 2023

Thanks for going into more details over this.

API surface

I think one risk that you highlight in your second message is that there might be a temptation to add more and more exports. For example, if getting stdout with o might be useful, so might be retrieving stdout + stderr, therefore requiring yet another export.

While convenience methods improve the developer experience by providing with shortcuts for common tasks, they also extend the API surface, making the documentation terser for new users. It's a trade-off.

Parallel commands

Another thing is that synchronous methods can be an anti-pattern that we might want to discourage.

One would probably argue that in a typical script that runs commands one after the other, holding the CPU is not a problem at all, which is true. However, running commands in parallel can provide with a big performance improvement, and it can only be done asynchronously. In my experience, most typical scripts can run at least some of their commands in parallel, and it is often overlooked by developers used to sync methods.

One could then probably argue that users can always use both the synchronous and asynchronous methods. However, I have seen in practice that it can be hard to introduce async calls in more complex scripts, with many levels of nested functions. For example, let's assume a script calls a function A() which calls B() which calls C(), all synchronous. Then, if one wants to make two operations run in parallel in C(), all three functions must be turned into async ones. I remember some experiences at work when such problem led to big refactoring, and I had wished async methods had been used from the start.

Some scripts also use logic that runs in the background (such as setInterval(), setTimeout(), or even servers in some cases). Less experienced users might not realize that their synchronous calls are holding up those other tasks.

Due to this, preferring async calls over sync can be seen a practice we might want to encourage.

Shortcuts

That being said, I do agree with you that one of the design goals of the $ interface is simplicity. In that context, providing with the shortcuts you are mentioning might make sense, although it does require users to remember that $ is async and s is not (which is not completely obvious).

Retrieving stdout

Also, I agree that retrieving stdout is a very common use case.

Execa does provide with a convenient ${...} syntax which uses the stdout. But it only works when using a command's stdout as input to another. In some cases though, one needs to directly manipulate stdout instead (like in the example you provide).

Also, Bash provides with built-in syntax for this with $(...) (although this retrieves stderr too), which is very commonly used in shell scripts.


So, I think there are quite a few pros and cons to consider. @sindresorhus What do you think about this?

@sindresorhus
Copy link
Owner

As I said in the original post, I know that I can do this for my own projects by wrapping $.sync exactly like you showed in your reply.

You said "wrapping". I presented aliasing, which is not the same thing.

@sindresorhus
Copy link
Owner

I'm warming up to the idea, but I will need some time to think about it. If we do this, we would only do sync. No other alias, like output.

@sindresorhus
Copy link
Owner

These are the possible alternatives:

s`dep deploy`;

_`dep deploy`;

$_`dep deploy`;

_$`dep deploy`;

$$`dep deploy`;

@sindresorhus
Copy link
Owner

One alternative is this, but it also makes scripts using execa inconsistent, so I'm not a big fan:

import {$} from 'execa/sync';

const branch = $`git branch --show-current`.stdout;
if (branch === "main") {
  // Do something.
}

@Zamiell
Copy link
Author

Zamiell commented Oct 29, 2023

Thank you ehmicky for the detailed and thoughtful reply! I agree with you that there are pros and cons.

One alternative is this, but it also makes scripts using execa inconsistent, so I'm not a big fan

Yeah, I don't think that approach is very good, because it prevents using both sync and async at the same time. I have found that in my scripts, I want to use both. (e.g. I want to use sync if I am at the beginning of the script for "prep" work, and I want to use async at the end of the script for tasks that are not order-dependent.)

@Zamiell
Copy link
Author

Zamiell commented Oct 29, 2023

These are the possible alternatives:

Yes, it seems that the only two symbols that are allowed in JavaScript/TypeScript as functions are $ and _.

I think using _ is not good, as it is already kind of an overloaded symbol from the Underscore.js and Lodash libraries. In other words, it might be a bit confusing looking at a script with underscores, as someone might think to themselves: "Why are they attaching a template string to the Lodash library?"

Another alternative you didn't mention is:

s$`dep deploy`

$s`dep deploy`

Personally, I think is a bit more clear than packing on more symbols. However, I'll note that I still prefer a one-letter solution like s. =p

By the way, I think that if we all converge on s, it would also be nice to re-export $ as a, so that way a script with a good mix of sync and async can be more clear, e.g.:

s`npx tsc`
a`npx eslint .`
a`npx prettier .`

versus:

s`npx tsc`
$`npx eslint .`
$`npx prettier .`

Here, I think the former snippet with the two different letters is more easily understandable.

@sindresorhus
Copy link
Owner

By the way, I think that if we all converge on s, it would also be nice to re-export $ as a, so that way a script with a good mix of sync and async can be more clear, e.g.:

I don't think that's needed. The await makes it clear:

s`npx tsc`
await $`npx eslint .`
await $`npx prettier .`

I don't really want to export more aliases.

@sindresorhus
Copy link
Owner

One problem with s$ is that it's easy to forget which order to use it: s$ or $s.

@Zamiell
Copy link
Author

Zamiell commented Oct 29, 2023

I like $s over s$, since it is a bit easier to understand that the former is an alias for $.sync. Also, the symbol acts as a kind of namespace, e.g. I think it is conventional to make functions like foo() & fooWithSomeKindOfModifier(). Additionally, you could consider Node.js standard library functions like fs.exists() & fs.existsSync(), which lend towards $s to match this pattern.

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 29, 2023

I also think $s would be the best here, for the reason you mention. It make more sense to start with the same prefix. It is also clear with $ vs $s which one is sync and async, as opposed to $ vs s where this is unclear.
I would also consider it being one character longer a good thing, since sync calls should usually be discouraged.

I also agree that import {$} from 'execa/sync' would discourage using both sync and async calls in the same script (which is not good) since it would require more verbose imports.

import {$} from 'execa';
import {$ as $s} from 'execa/sync';

@sindresorhus
Copy link
Owner

Ok. Let's go with $s.

@aaronccasanova
Copy link
Contributor

aaronccasanova commented Nov 3, 2023

I'm a little late to the party, but curious what folks think about just including a shorter alias on $?

e.g. $.s`echo hello`

Would make the code updates trivial and not require adding a new entry point e.g.

$.sync = ...
$.s = $.sync

Also, $s LGTM 😄

@ehmicky
Copy link
Collaborator

ehmicky commented Nov 3, 2023

Thanks for chiming in @aaronccasanova. For memories, @aaronccasanova suggested and implemented the $ feature (which is awesome, thanks again!).

I would personally favor $.s over $s, but either works. Thoughts @sindresorhus?

Another question would be: should $.sync be removed (breaking change), deprecated (documentation-only), or kept as is? I would personally favor deprecation, followed by removal in next breaking change. No need to have two names for the same method.

@sindresorhus
Copy link
Owner

I'm in favor of $.s.

@sindresorhus
Copy link
Owner

Another question would be: should $.sync be removed (breaking change), deprecated (documentation-only), or kept as is? I would personally favor deprecation, followed by removal in next breaking change. No need to have two names for the same method.

I would keep $.sync for now. I personally prefer the verbose name. But we can revisit whether to deprecate it later on.

@ehmicky
Copy link
Collaborator

ehmicky commented Dec 14, 2023

Done in #594.

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 29, 2024

One thing to consider for anyone using $.s is that it lacks lots of features (see #936) compared to await $.

Overall, it seems to me that synchronous invocations are useful only in very specific contexts (such as in a beforeExit event handler, or in any callback that cannot be async). Script invocation is a commonly mentioned context where it should probably not be used, as it discourages parallelizing commands.

Overall, we probably should discourage using $.sync(), and giving it a shorter alias is the opposite.

@ehmicky
Copy link
Collaborator

ehmicky commented May 8, 2024

This feature has been just released in Execa 9.0.0. Please see (and share!) the release post and the changelog.

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

Successfully merging a pull request may close this issue.

4 participants