fix(deps): bump execa to the latest version (5.x) #3440
Conversation
| // The child process `error.message` includes stderr and stdout output which most of the times contains duplicate | ||
| // information. We rely on `error.shortMessage` instead. | ||
| error.message = error.shortMessage |
There was a problem hiding this comment.
Related with sindresorhus/execa#397 which I imagine you're familiar with @ehmicky 😅
Keeping the current behaviour resulted in extra output in our snapshots, usually with the same kind of information reproduced twice. Wondering what you think?
There was a problem hiding this comment.
Yes, this makes sense 👍
I answered below at #3440 (comment)
packages/git-utils/src/exec.js
Outdated
| const { stdout } = execa.sync('git', args, { cwd: cwdA }) | ||
| return stdout | ||
| // The child process `error.message` includes stderr and stdout output which most of the times contains duplicate | ||
| // information. We rely on `error.shortMessage` instead. |
There was a problem hiding this comment.
In this instance, wouldn't the git output be useful for debugging purpose?
There was a problem hiding this comment.
The message we're getting contains the exact same input twice. Sharing the snippet so you can see:
## Git utils fails if no root
> Snapshot 1
`␊
────────────────────────────────────────────────────────────────␊
Netlify Build ␊
────────────────────────────────────────────────────────────────␊
␊
> Version␊
@netlify/build 1.0.0␊
␊
> Flags␊
debug: true␊
repositoryRoot: /tmp-dir␊
testOpts:␊
pluginsListUrl: test␊
silentLingeringProcesses: true␊
␊
> Current directory␊
/tmp-dir␊
␊
> Config file␊
/tmp-dir/netlify.toml␊
␊
> Resolved config␊
build:␊
publish: /tmp-dir␊
publishOrigin: default␊
plugins:␊
- inputs: {}␊
origin: config␊
package: ./plugin␊
␊
> Context␊
production␊
␊
> Loading plugins␊
- ./plugin from netlify.toml␊
␊
────────────────────────────────────────────────────────────────␊
1. onPreBuild command from ./plugin ␊
────────────────────────────────────────────────────────────────␊
␊
␊
────────────────────────────────────────────────────────────────␊
Plugin "./plugin" internal error ␊
────────────────────────────────────────────────────────────────␊
␊
Error message␊
Error: Invalid head commit HEAD␊
Command failed with exit code 128: git rev-parse HEAD␊
fatal: not a git repository (or any of the parent directories): .git␊
fatal: not a git repository (or any of the parent directories): .git␊
␊
Error location␊
In "onPreBuild" event in "./plugin" from netlify.toml␊
STACK TRACE␊
␊
Resolved config␊
build:␊
publish: /tmp-dir␊
publishOrigin: default␊
plugins:␊
- inputs: {}␊
origin: config␊
package: ./plugin`
My understanding is that in order to keep the current behaviour we'll need to rely on shortMessage as the error message has the same input contained in stdout/stderr?
| @@ -38,6 +38,9 @@ const coreCommand = async function ({ | |||
| handleBuildCommandOutput(buildCommandOutput, logs) | |||
| return {} | |||
| } catch (error) { | |||
| // The child process `error.message` includes stderr and stdout output which most of the times contains duplicate | |||
| // information. We rely on `error.shortMessage` instead. | |||
There was a problem hiding this comment.
I am wondering whether we should mention instead that the build command stdout/stderr is:
- Streamed in production, i.e.
message === shortMessagethen (since stdout/stderr cannot be directly buffered when streamed) - Buffered in
logsin tests, but we already printlogsso we don't need to re-print them
There was a problem hiding this comment.
Is it a matter of streaming? 🤔 I just assumed this was related with the message field contents, which we always log(?). Anyway happy to rephrase the comment to something that makes more sense 👍
There was a problem hiding this comment.
In production (i.e. when buffer: false CLI flag is used), we use stdio: 'inherit' on the build command. This prevents Node.js child_process from being able to buffer stdout/stderr and as a consequence, error.message should not contain stdout/stderr and be equal to error.shortMessage.
In tests, we pass buffer: true so that logs are concatenated to a logs array instead of being printed on stdout/stderr. We also then pass stdio: 'pipe' on the build command, which allows buffering. At the end of each test, we print the contents of those logs. I.e. without error.shortMessage, we would end up printing the build command output twice.
There was a problem hiding this comment.
Ah makes perfect sense, thanks for taking the time to explain it. I've updated the comment with what you've just shared in 75fbf28 🙌
JGAntunes
force-pushed
the
chore/update-execa
branch
from
5e4ca22
to
5847e21
Compare
JGAntunes
force-pushed
the
chore/update-execa
branch
from
c1434f9
to
75fbf28
Compare
JGAntunes
added
automerge
type: bug
Which problem is this pull request solving?
Follow up of #3321, we can now bump execa to 5.x. see - https://github.com/sindresorhus/execa/releases. I figure @ehmicky probably has some useful input to give here 👀
Checklist
Please add a
xinside each checkbox: