Exit code from depcheck executable

[domharrington]

Using the depcheck executable should exit with an error code if there are unused dependencies. Running it on my project this doesnt appear to be the case.

$ depcheck
Unused devDependencies
* gulp-eslint
* should-http
Missing dependencies
* mongoose-text-search

Results in an exit code:

$ echo $?
0

Versions:
Node: v5.7.0
depcheck: 0.6.3.

I've had a look around in the code and the issue appears to stem from this block of code:

depcheck/bin/depcheck

Lines 11 to 13 in 6cb9253

	process.stdout.on('finish', function finish() {
	process.exit(code);
	});

If you move the process.exit to outside of the on('finish') callback then it works as expected. Have you seen this issue before? What was the reasoning behind adding that callback?

Edit:
This was introduced here: 44124dd

Checking out the version before the 0.6.0 release (0.5.11) has fixed the issue.

Edit 2:
I've added a regression test for this issue in my fork:
domharrington@99e6ce1
Any help would be appreciated!

Edit 3:
According to the node docs, process.stdout never emits a finish event: https://nodejs.org/api/process.html#process_process_stdout. Can confirm this is the case as when i put a log statement into that callback, it never gets called.

[lijunle]

Hi, @domharrington thanks for your awesome investigation! I will take a look on your code later.

@gtanner, do you have any ideas about this?

[iamstarkov]

@lijunle i guess it can be fixed by using process.exitCode = 1;

[domharrington]

I'm not sure that it can... because the callback https://github.com/depcheck/depcheck/blob/6cb92533adf3ed2ed3f6054d6cb3c51e56731615/bin/depcheck#L12 never gets called. We can just remove this callback, but then i'm not sure what issues this was causing in #98

[iamstarkov]

we can remove this callback all together, and just set process.exitCode = 1 in the cli if there is anything unused or in case of error

[iamstarkov]

are there any tests covering required behaviour?

[lijunle]

I saw this pork request to do similar thing in npm-check. It looks like we can do the same thing here.

@iamstarkov Yes, there will be treat case cover the exit code. But I think the test codes need to update too, if we are going to remove the process.exit call.

[lijunle]

And, for @domharrington 's requirement, check the update 2 from the first post. That test is very nice, should be cherry picked to here. :)

[lijunle]

Found this package and it will handle the stdout for us: https://www.npmjs.com/package/exit

[lijunle]

Fixed with the awesome process.exitCode. Thank you, all! 👏

[iamstarkov]

cool

[pdehaan]

Any chance we get get a new release published to npm with this fix?

[gflarity]

Publish please :)

[domharrington]

Can you publish a new version to npm?

[lijunle]

@domharrington The version 0.6.4 should include this fix. Could you please test it?

[PatrickEickmeier]

HI @lijunle I use version 0.6.7 and it doesn't work for me.

[PatrickEickmeier]

I made some tests and it seems that there is only an exit code >0 when there are unused dependencies.

$ depcheck --ignores=winston
Missing dependencies
* handlebars
$ echo $?
0

$ depcheck --ignores=handlebars
Unused dependencies
* winston
$ echo $?
255