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

Broken CLI Thanks To COA & JavaScript Hell #739

Closed
bdkjones opened this issue Jun 10, 2017 · 4 comments
Closed

Broken CLI Thanks To COA & JavaScript Hell #739

bdkjones opened this issue Jun 10, 2017 · 4 comments

Comments

@bdkjones
Copy link

Looking through your source, it seems like you use this thing to handle your CLI option parsing.

I just reinstalled SVGO 0.7.2 and npm pulled down COA 1.0.2. This seems to have introduced the following bug:

The Bug

Running svgo -v at the command line will now return an exit code of 1. This may not SEEM like a big deal, but if you happen to be integrating SVGO into another tool, a non-zero exit code means, "Something went wrong; the program did not run successfully." The correct exit code is super important and, here, it should absolutely be 0 because SVGO successfully returned a version.

It's Definitely COA

The reason I think this is COA's fault is that you already have a workaround in your code to log Version output to stdout instead of stderr and a comment indicating this is a workaround for COA.

Additionally, an older installation of SVGO 0.7.2 works just fine and that installation has COA 1.0.1. When I used COA 1.0.1 in the new installation, the problem went away.

Discussion

I'm not sure what's changed, exactly, but it looks like COA wasn't under active development for a while and just pushed through a bunch of changes (looking at their commit history).

Perhaps lock the dependency to COA 1.0.1? Or move to a different library? Or perhaps your workaround is no longer needed with COA 1.0.2 because they cleaned up their end, so now your workaround is actually doing the reverse of what's correct? I dunno... it's 1AM here and I'm not JavaScript's biggest fan.

@bdkjones
Copy link
Author

Also, that workaround I mentioned is in coa.js here:

screen shot 2017-06-10 at 01 11 09

@qfox
Copy link

qfox commented Jun 10, 2017

Please try to reinstall svgo, this should be fixed in COA v1.0.3

@qfox
Copy link

qfox commented Jun 10, 2017

Workaround could be dropped now, it's true.

ps.
Looks like I should clarify what happened with COA and why it wasn't under active development.

COA was originally written on CoffeeScript and since nobody want to write code on Coffee there were no activity.
About a month ago we decided to rewrite COA to Node 4+ and ES6 classes because we love how COA was designed, it's API and we have a bunch of ideas to evolute.
Since we shouldn't change anything in the public API, we decided to publish it under 1.0.2.

Unfortunately COA has like 70% lines coverage and practically there are a lot of uncovered edge-cases.

Sorry that it affected you in CI like that ;-(

@TrySound
Copy link
Member

Coa is replaced with commander

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants