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(core): drain stdout before exit in print-affected #12559

Merged

Conversation

forivall
Copy link
Contributor

Piping a large print-affected output to another command, such as cat or jq, resulted in broken json output as the node process can exit before the output is fully written. The process.stdout stream needs to wait to drain before process.exit.

Current Behavior

Sometimes, when I pipe nx print-affected --all to another command, such as cat or jq, the output will be truncated. For example:

~pubrepos/nx forivall/print-affected-stdout-drain ≡
» nx print-affected --all | cat
{
  "tasks": [],
  "projects": [
    "nx-dev-feature-package-schema-viewer",
    "make-angular-cli-faster",
...
          "type": "static"
        }
      ],
      "nest": [
        {
          "source": "nest",
          "target": "node",
          "type": "impli
~pubrepos/nx forivall/print-affected-stdout-drain ≡
» nx print-affected --all | jq .
parse error: Unfinished JSON term at EOF at line 3479, column 3

Expected Behavior

» nx print-affected --all | jq .
{
  "tasks": [],
  "projects": [
    "nx-dev-feature-package-schema-viewer",
    "make-angular-cli-faster",
    "create-nx-workspace",
...

Related Issue(s)

I don't think this issue has been reported yet.

Fixes #

@vercel
Copy link

vercel bot commented Oct 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nx-dev ✅ Ready (Inspect) Visit Preview Oct 12, 2022 at 10:15PM (UTC)

Comment on lines +273 to +277
if (process.stdout.writableNeedDrain) {
process.stdout.once('drain', resolve);
} else {
resolve();
}
Copy link
Contributor Author

@forivall forivall Oct 12, 2022

Choose a reason for hiding this comment

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

alternative:

Suggested change
if (process.stdout.writableNeedDrain) {
process.stdout.once('drain', resolve);
} else {
resolve();
}
process.stdout.write('', resolve);

Notably, writableNeedDrain was added in node 14.17.0, so on earlier node versions, the code that checks writableNeedDrain won't wait for drain; that is the current behaviour, so that's somewhat moot.

Piping a large print-affected output to another command, such as cat or jq, resulted in broken json
output as the node process can exit before the output is fully written. The process.stdout stream
needs to wait to drain before process.exit.
@FrozenPandaz FrozenPandaz merged commit 0ff93dd into nrwl:master Oct 12, 2022
FrozenPandaz pushed a commit that referenced this pull request Oct 14, 2022
Piping a large print-affected output to another command, such as cat or jq, resulted in broken json
output as the node process can exit before the output is fully written. The process.stdout stream
needs to wait to drain before process.exit.

(cherry picked from commit 0ff93dd)
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants