Nativescript-cli: CLI does not respect shelljs errors

[rosen-vladimirov]

Shelljs module does not fail in case shelljs.config.fatal is not set to true. However setting it true leads to some unexpected issues - the shelljs silently exits the process when calling tns platform add [email protected] for example.
So consider respecting the errors of shelljs, but make sure all operations work.

[slavchev]

I think we should increase the priority of this issue as I was debugging entirely different problem only to find that there were silent/missing error handling.

[Plamen5kov]

One solution to this problem is checking each individual place we use shelljs and figure out what behavior we want to have. This way we'll have full control over shelljs behavior.

[rosen-vladimirov]

@Plamen5kov in this case I would recommend extracting all shelljs calls in a separate class that will be resolved via $injector.
The checks for errors will be only in this class. This will improve the error handling and will make the current code of CLI more testable.

[Plamen5kov]

@rosen-vladimirov I'm still struggling to see the best possible solution, because we need to change the behavior depending on the place of usage. We might have a scenario, where we have and expected error and can continue execution, and a case where the same command needs to have different consequences. It's hard to see a general solution that will cover all problems. It's a good idea to wrap shelljs command, but that won't help with individual logic we have to implement on shelljs failure.

Another option is to log every error, but continue execution. That way we at least shot what possible errors might have caused a failure.

[rosen-vladimirov]

@Plamen5kov I think in most of the cases we would like to stop the execution in case shelljs encounters an error. However (as far as I remember), setting shelljs.config.fatal breaks the process immediately, so we have no chance to catch this. My suggestion is:
1) Implement a wrapper for shelljs:

class Shelljs implements IShelljs {
...
}
$injector.register("shelljs", shelljs);

2) Add a method in the new class, that executes shelljs action, checks for error and throws it in case there's any error:

import * as shelljs from "shelljs";

class Shelljs implements IShelljs {
    constructor(private $logger: ILogger) { }
    public executeAction(command: string, commandArgs: string[]): any {
        const result = shelljs[command].apply(null, commandArgs);
        const err = shelljs.error();
        if (err) {
            this.$logger.trace("Encountered shelljs.error: ", err);
            throw new Error(err);
        }

        return result;
   }
}
$injector.register("shelljs", Shelljs);

3) Use the method anywhere you need shelljs. In case on a specific place you do not want to fail, you can try-catch the code there and continue the execution. Another option is to pass additional flag to the executeAction method.

// This will fail in case error is encountered:
this.$shelljs.executeAction('cp', ["-r", "./my-dir", "./my-dir2"]);

// This will not fail in case error is encountered:
try {
    this.$shelljs.executeAction('cp', ["-r", "./my-dir", "./my-dir2"]);
} catch (err) {
    // do smth with the error
}