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

fix: Exit status code for fatal Webpack exceptions #1754

Merged
merged 4 commits into from
Dec 17, 2022
Merged

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Dec 15, 2022

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

No

Summary

Closes #1744

We don't correctly handle fatal Webpack exceptions at the moment; the err in the callback for compiler.run() relates to fatal Webpack exceptions, like configuration errors. When one is hit, we only reject the promise, which actually allows the build to "complete" with a status code of 0. We instead want to exit immediately with a status code 1 (as that matches what we do for compilation errors).

Does this PR introduce a breaking change?

No

@rschristian rschristian requested a review from a team as a code owner December 15, 2022 02:40
@changeset-bot
Copy link

changeset-bot bot commented Dec 15, 2022

🦋 Changeset detected

Latest commit: 42b1458

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines -83 to +88
showStats(stats, true);

if (err || (stats && stats.hasErrors())) {
rej(`${red('\n\nBuild failed! \n\n')} ${err || ''}`);
if (err) {
error(err, 1);
}

showStats(stats, true);

Copy link
Member Author

Choose a reason for hiding this comment

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

showStats(), in production builds, will also immediately exit upon coming across any errors; as such, there's no reason to ever reject this promise as we've already exited.

@@ -16,6 +16,7 @@ jobs:
test:
name: Test
runs-on: ubuntu-latest
timeout-minutes: 10
Copy link
Member Author

@rschristian rschristian Dec 15, 2022

Choose a reason for hiding this comment

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

Unrelated, but for some (godforsaken) reason, puppeteer randomly never exits on the CI. I've seen no pattern for this, just that it occurs.

GitHub's default is 360 minutes, or 6 hours, which we definitely don't need. I just caught a run still going after an hour and had to manually kill it. 10 minutes should be plenty of time.

@rschristian rschristian marked this pull request as draft December 15, 2022 05:08
@rschristian rschristian marked this pull request as ready for review December 15, 2022 05:52
"puppeteer": "^9.1.1",
"puppeteer": "^13.7.0",
Copy link
Member Author

@rschristian rschristian Dec 15, 2022

Choose a reason for hiding this comment

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

This shouldn't be necessary, but our version of Puppeteer has turned super flakey recently. Updating seems to make it somewhat less so.

@rschristian rschristian merged commit 227bfb7 into master Dec 17, 2022
@rschristian rschristian deleted the fix/status-code branch December 17, 2022 20:33
@preact-bot preact-bot mentioned this pull request Dec 17, 2022
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.

Failed builds return Exit Code 0
1 participant