Skip to content

Fixing postcss not exiting with the proper exit code in --watch#178

Merged
RyanZim merged 2 commits intopostcss:developfrom
GreenGremlin:watch-exit-code
Dec 6, 2017
Merged

Fixing postcss not exiting with the proper exit code in --watch#178
RyanZim merged 2 commits intopostcss:developfrom
GreenGremlin:watch-exit-code

Conversation

@GreenGremlin
Copy link
Copy Markdown
Contributor

When running postcss --watch, if an error is encountered during the first pass postcss currently exits with an exit code of 0. The reason this is happening is because the error function specifically doesn't call process.exit(1) in watch mode.

@RyanZim
Copy link
Copy Markdown
Collaborator

RyanZim commented Nov 30, 2017

error() is hard-coded to process.exit(1): https://github.com/GreenGremlin/postcss-cli/blob/master/index.js#L294

@GreenGremlin
Copy link
Copy Markdown
Contributor Author

Except for this if (argv.watch) return. In watch mode it returns before calling process.exit(1).

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

In case you got a SyntaxError postcss-clidoesn't exit watch mode https://github.com/GreenGremlin/postcss-cli/blob/master/index.js#L290

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

michael-ciniawsky commented Nov 30, 2017

Yep exactly, that's intended

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

The CLI should recover from SyntaxErrors as these errors tend to happen during development, while a CLI/ Config Error normally is FATAL => process.exit(1)

@GreenGremlin
Copy link
Copy Markdown
Contributor Author

A better solution would be to resolve with the list of files and not exit, but this is an easier change, for now.

@RyanZim
Copy link
Copy Markdown
Collaborator

RyanZim commented Nov 30, 2017

Duh, yeah forgot about that line. That's totally intended; not changing this.

@RyanZim RyanZim closed this Nov 30, 2017
@GreenGremlin
Copy link
Copy Markdown
Contributor Author

Can you explain why? This is an issue for me. I have a script that is running babel and postcss, both in watch mode, and I need to know if postcss exits with an error.

@michael-ciniawsky
Copy link
Copy Markdown
Contributor

As mentioned, your solution would by a undesired breaking change since it breaks watch mode on SyntaxError and simply isn't justifiable to implement in the scope of this project for suiting your particular use case. If you wouldn't insists on letting babel handle File I/O for postcss-cli you will see the benefit of the current logic at last when accidentally making a typo will editing some CSS during development in watch mode yourself

@GreenGremlin
Copy link
Copy Markdown
Contributor Author

My change doesn't break watch mode, as I'm only calling process.exit(1) when an error is encountered during the first pass. Watch mode already exits when there's a syntax error during the first run, but it is exiting with an exit code of 0.

Also, babel isn't handling file I/O, it just happens to be running at the same time as postcss. With my change postcss does not exit if you accidentally make a typo.

@RyanZim
Copy link
Copy Markdown
Collaborator

RyanZim commented Dec 1, 2017

Ah, sorry, misunderstood your change. Yeah, for the first pass, that's debatable. Reopening for discussion.

@RyanZim RyanZim reopened this Dec 1, 2017
@postcss postcss deleted a comment from coveralls Dec 1, 2017
@postcss postcss deleted a comment from coveralls Dec 1, 2017
@postcss postcss deleted a comment from coveralls Dec 1, 2017
Copy link
Copy Markdown
Contributor

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's a bug the process should immediately exit with the correct process.exitCode = 1 in this case (first pass) imho

Comment thread index.js
})
.catch(error)
.catch(err => {
error(err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\n

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 1, 2017

Coverage Status

Coverage decreased (-0.3%) to 75.497% when pulling c7406f6 on GreenGremlin:watch-exit-code into b4d5d3d on postcss:master.

Copy link
Copy Markdown
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is semver-major, will be a bit till I release it.

@postcss postcss deleted a comment from coveralls Dec 5, 2017
@postcss postcss deleted a comment from coveralls Dec 5, 2017
@RyanZim RyanZim changed the base branch from master to develop December 6, 2017 00:00
@RyanZim RyanZim merged commit 5968ac7 into postcss:develop Dec 6, 2017
@RyanZim
Copy link
Copy Markdown
Collaborator

RyanZim commented Dec 6, 2017

Merged into develop, will be in the next major.

@RyanZim
Copy link
Copy Markdown
Collaborator

RyanZim commented Feb 7, 2018

Released in v5, sorry for the long wait here.

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

Successfully merging this pull request may close these issues.

4 participants