fix(bin): write into streams before exit

[mmeineke]

I run into an issue, that the output from react-docgen is cut off, if the output is too big.
This only happens if I try to pipe the output like so:

react-docgen ./components | node ./buildDocs.js

Or like so:

let spawn = require('child_process').spawn;
let child = spawn('react-docgen', ['./Components']);
let buffers = [];
child.stdout.on('data', data => {
    buffers.push(data);
});
child.stdout.on('close', () => {
    let buffer = Buffer.concat(buffers);
    console.log(`${buffer}`);
});

After some research I found this issue:
mochajs/mocha#333
And it works with the solution they used:
https://github.com/mochajs/mocha/pull/1440/files

So hopefully you can use this fix. I will try to help where I can to get it merged. Another solution could be this package: https://www.npmjs.com/package/exit

[danez]

With which node version do you have the problem? As far as I see it was fixed in node 0.12.

[mmeineke]

I tried it with node 4 and 6. Was wondering as well why I ran into this issue

[mmeineke]

I will provide you with an example

[mmeineke]

I created 2 examples which uses exactly the same code as in the examples in this repo beside the amount of props. The first example with a component with 100 props and the second with 1000 props. Please try if the second one fails for you as well.
https://github.com/mmeineke/react-docgen-test

[danez]

Thanks, yes I can reproduce this.

Reading more about writing to stdout I found this in the docs:

The process.stderr and process.stdout streams are blocking when outputting to TTYs (terminals) on OS X as a workaround for the operating system's small, 1kb buffer size. This is to prevent interleaving between stdout and stderr.

To check if Node.js is being run in a TTY context, check the isTTY property on process.stderr, process.stdout, or process.stdin.

For instance:

$ node -p "Boolean(process.stdin.isTTY)"
true
$ echo "foo" | node -p "Boolean(process.stdin.isTTY)"
false

So as you are piping we are not in a tty anymore which makes the stream write non-blocking and the process.exit() immediately afterwards might interrupt the writing.

Instead of doing this draining logic, I would more lean to remove process.exit() from line 98 and 110 and replace the first one with process.exitCode = 1;. The process is ending after the output anyway, I don't see a need to manually do the exit call.

@fkling What do you think?

[fkling]

Instead of doing this draining logic, I would more lean to remove process.exit() from line 98 and 110 and replace the first one with process.exitCode = 1;.

If that works, I'm all for it since it's the simpler solution. However, we are invoking exitWithError in line 142 which is the async directory traversal process. Maybe we can just return there instead?

[mmeineke]

Hey, I removed the process.exit completely now.

• renamed exitWithResult to writeResult
• removed exitWithError (added process.exitCode = 1; instead)
• traverseDir throws an error now which gets catched

What I don't like and hopefully you have a better idea is line 153:

if (process.exitCode !== 1) {

Which I needed to implement because of the removed exitWithResult in line 91/95

[mmeineke]

@danez , @fkling what do you think?

[fkling]

Couldn't we just call process.exit(1) here?

[mmeineke]

Actually yes, but I wanted to get rid of the eslint no-process-exit: 0 line :D
I would say you should only disable a linting rule if there is no other way.

[fkling]

Well, that rule is more targeted towards arbitrary modules. From the docs:

This is a dangerous operation because it can occur in any method at any point in time, potentially stopping a Node.js application completely when an error occurs.

So, using it in the executable file itself is completely valid :)

[mmeineke]

Seems legit. But disabling this rule could lead to the same problem in the future.
So I would prefer to have this rule enabled also here. Are the new changes ok for you?

[fkling]

You can always disable the rule for just that one line ;)

But yeah, I guess this is OK.

[mmeineke]

I replaced the writeError+ process.exit with an errorMessage which could be used for other error messages in the future as well.
This errorMessage gets written below with the process.exit.
Would be fine for me now.

[mmeineke]

@danez Hey, could we merge the PR now?

[danez]

lgtm

[mmeineke]

@danez thanks a lot :)

[fkling]

Thanks for this! I'll release a new version shortly.