search

ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.7k stars 244 forks source link

Fix missing exit code on build script failure #523

Closed xzyfer closed 6 years ago

xzyfer commented 6 years ago

Currently the build command always exits with a 0 code when a compilation error is encountered which prevents compilation failures failing builds in CI environments.

The err object will not include compilation errors and those must be handled separately using stats.hasErrors() [...]. The err object will only contain webpack-related issues, such as misconfiguration, etc. webpack node API documentation

This PR sets the exit code to 1 when an error is encountered but lets the process exit naturally rather than a force exit, which can be problematic when async process are in play.

This has benefits over for exiting the process i.e. not exiting when watching. It also doesn't require adding additional or cli flags.

Additionally this preserves non-zero exit codes if they've already been set to preserve BC.

The specific implementation is inspired by Webpack's Node API error handling documentation with considerations made for maintaining BC.

ctrlplusb commented 6 years ago

Zing, great catch @xzyfer!

ctrlplusb commented 6 years ago

@xzyfer would you mind rebasing this against next? I am ready to merge but would like it to go into next as we are queuing up a few big updates there.

xzyfer commented 6 years ago

Done :)

ctrlplusb commented 6 years ago

Hi @xzyfer this PR is still showing a lot of the changes that were already added to next. I think it might be easier for you to create a new branch off of next and then create a new PR with your change here: https://github.com/ctrlplusb/react-universally/pull/523/files#diff-5dbb54beea22fab8ea3a4af3ec9b96a6

Otherwise I am happy to apply the code change directly to next for you.

Let me know :)

xzyfer commented 6 years ago

Rebased on next

ctrlplusb commented 6 years ago

Perfect thanks 🤘

ctrlplusb commented 6 years ago

Should we call process.exit with the exit code explicitly?

xzyfer commented 6 years ago

The intent was to not override a non-zero exit code igniter was already set by something else.

ctrlplusb commented 6 years ago

Yeah, sorry I realised this after I wrote the comment, but should we not just call process.exit with the error code immediately rather than set the process.exitCode?

xzyfer commented 6 years ago

This docs suggested force exiting may not always be safe. Given that the guard here prevents extra work happening after the exitcode is set it seemed fine to just let the process terminate naturally.

ctrlplusb commented 6 years ago

TIL! 👍