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

${childProcessResult} shortcut notation #523

Closed
ehmicky opened this issue Mar 1, 2023 · 6 comments · Fixed by #528
Closed

${childProcessResult} shortcut notation #523

ehmicky opened this issue Mar 1, 2023 · 6 comments · Fixed by #528

Comments

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 1, 2023

zx has a ${childProcessResult} shortcut notation:

const branch = await $`git branch --show-current`
await $`dep deploy --branch=${branch}`

This is equivalent to:

const branch = await $`git branch --show-current`
await $`dep deploy --branch=${branch.stdout.replace(/\n$/, '')}`

This is quite handy and would be minimal to implement for us as part of the new $ API.
What do you think @sindresorhus @aaronccasanova?

@ehmicky ehmicky changed the title ${childProcess} shortcut notation ${childProcessResult} shortcut notation Mar 1, 2023
@aaronccasanova
Copy link
Contributor

Agreed, this would be a nice and subtle DX improvement. Two initial thoughts:

  1. Is there currently anything to key off of and identify the return value is a childProcessResult?
  2. Would this only work for string encodings? e.g.
const branch = await $({encoding: null})`git branch --show-current`
await $`dep deploy --branch=${branch}` // <- Would parseTemplates convert the buffer to a string?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 2, 2023

  1. Is there currently anything to key off of and identify the return value is a childProcessResult?

It is a plain object:

execa/index.js

Lines 141 to 152 in eae327c

return {
command,
escapedCommand,
exitCode: 0,
stdout,
stderr,
all,
failed: false,
timedOut: false,
isCanceled: false,
killed: false,
};

I think just checking if the value is a plain object with a stdout key might be enough?

  1. Would this only work for string encodings?

That's a great question! Considering the arguments are escaped and passed as is, I think we might allow buffer encodings? I.e. if Buffer.isBuffer(stdout), we would use stdout.toString(). One thing to note though is that child_process.spawn() throws if any argument contains a null byte.

However, if options.stdout is inherit, childProcessResult.stdout will be undefined. In that case, we might want to throw an error, as this mostly likely indicates a user error.


Additional note about stripping the final newline: it is already performed by default on stdout. The user can opt out of that behavior using the stripFinalNewline: false option. However, if they explicitly do so, this probably indicates that they do want the final newline. This means we probably do not need to perform any logic on stdout when it comes to the final newline: it is already good as is.

execa/index.js

Line 37 in eae327c

stripFinalNewline: true,

execa/index.js

Lines 68 to 70 in eae327c

if (options.stripFinalNewline) {
return stripFinalNewline(value);
}

@sindresorhus
Copy link
Owner

I think just checking if the value is a plain object with a stdout key might be enough?

That sounds fine. If we really want to ensure users don't pass in something incorrect, we can add an internal Symbol to the returned object and check for that.

@sindresorhus
Copy link
Owner

I like it 👍

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 2, 2023

@aaronccasanova Would you be interesting in implementing it? If not, no worries! :)

@aaronccasanova
Copy link
Contributor

I have bandwidth to help out with this update 👍 I'll likely need some assistance dialing in the right test cases, but I'll push up a draft implementation and we can go from there!

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 a pull request may close this issue.

3 participants