search

JamieMason / ImageOptim-CLI

Make optimisation of images part of your automated build process
https://foldleft.io/image-tools
MIT License
3.45k stars 125 forks source link

Exit with non-zero status code on error #180

Closed Nowaker closed 4 years ago

Nowaker commented 4 years ago

Steps To Reproduce The Error

Expected Behaviour

i Running ImageOptim...
! ImageOptim.app is not installed (https://imageoptim.com/mac)
! No size savings
✓ Finished

Exit status 1, indicating a failure. The ! ImageOptim.app is not installed line should be !!! and red to indicate an error.

Actual Behaviour

i Running ImageOptim...
! ImageOptim.app is not installed (https://imageoptim.com/mac)
! No size savings
✓ Finished

No error. Process exits with exit status 0, indicating a success. The ! lines are yellow, indicating a warning, not error.

Version Numbers or N/A

% imageoptim --version
2.3.9

Help Needed

What kind of help or information do you need to help you create a PR? This can help others understand what they can do to help you get started.

I'm not interested to create a PR.

jamesstout commented 4 years ago

I'm not interested to create a PR.

Just an FYI then?

@JamieMason maybe a different logger level?

export const notinstalled = (value: string): void => {
    console.log(color.red('!!! %s'), value);
    process.exit(1);
  };

Then change the warning() calls in the runners, e.g.:

if (!(await pathExists(IMAGEOPTIM_BIN_PATH))) {
    return notinstalled(`ImageOptim.app is not installed (${IMAGEOPTIM_URL})`);
}

Not sure if this means it would be caught by your exception handler, I don't think so... I think the Node process stops as soon as you call process.exit(1);

https://github.com/JamieMason/ImageOptim-CLI/blob/8a25ef91e97e7529add28f666fd6c9545f080266/src/index.ts#L60-L63

Seems a bit overkill for a rare scenario to me though.

JamieMason commented 4 years ago

Thanks @jamesstout, I think you're right that a new log level is the way to go – it seems better to throw than warn and it's easy for those who might not have been using --no-imageoptim (that didn't have ImageOptim installed) and were ignoring the warnings to just add --no-imageoptim to prevent the error and quit.

I think the Node process stops as soon as you call process.exit(1);

that's correct

jamesstout commented 4 years ago

that's correct

Thanks, so if bug() calls process.exit(1);, will this line ever run?

https://github.com/JamieMason/ImageOptim-CLI/blob/8a25ef91e97e7529add28f666fd6c9545f080266/src/index.ts#L62

[not sure what await does .. something something async promises?]

Nowaker commented 4 years ago

Thank you guys for looking into it!

I've just corrected the issue title. Not sure how it ended up like that.

Seems a bit overkill for a rare scenario to me though.

Not sure how rare it is but I used imageoptim on Linux for years, and now when I'm on Mac I installed it using brew and thought it was working (seeing success messages and all that) but it actually did not.

[not sure what await does .. something something async promises?]

await is a modern JS way to convert visually cluttered callbacks to code that looks iterative but still operates on callbacks. More on it here: https://blog.bitsrc.io/keep-your-promises-in-typescript-using-async-await-7bdc57041308

JamieMason commented 4 years ago

[not sure what await does .. something something async promises?]

that's right, it waits for the promise to resolve before continuing to the next line. Good thing you mentioned this as I'd missed the tearDown stuff, so I'll make sure that is always run.

jamesstout commented 4 years ago

Not sure how rare it is but I used imageoptim on Linux for years, and now when I'm on Mac I installed it using brew and thought it was working (seeing success messages and all that) but it actually did not.

Yes, apologies .. I was thinking only in macOS terms, and I agree, a correct return code is needed.

await is a modern JS way to convert visually cluttered callbacks to code that looks iterative but still operates on callbacks.

Thanks. I'm a JavaScript novice trying and failing to get this TypeScript project to build 😢

jamesstout commented 4 years ago

Good thing you mentioned this as I'd missed the tearDown stuff, so I'll make sure that is always run.

Definitely doesn't run clean() (at the moment) when it exits on error:

Screen Shot 2019-09-16 at 4 03 16 AM

JamieMason commented 4 years ago

I realised after releasing 😬 and started this change in a dev branch https://github.com/JamieMason/ImageOptim-CLI/commit/5a6b1d4f2977131b6b12b5ee33d51952b7caba81