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

Ensure a non-zero exit status code when build has errors #1451

Merged
merged 2 commits into from Jun 5, 2020

Conversation

nddery
Copy link
Collaborator

@nddery nddery commented Jun 3, 2020

Description

Ensure init() always return a Promise. This prevents the issue described in #1260.

Since init() returns a Promise, it would need to be awaited for. However, we can't await at the top-level, so we would have needed to wrap the try/catch in an async function for it to work, like so:

Example of awaiting:

async function main() {
  try {
    await init()
  } catch(e) {
    console.trace(e)
    process.exit(1)
  }
}
main()

This just seem overly complex, so I went for the catch as it's nicer than wrapping the try/catch.

Changes/Tasks

  • Changed code

  • yarn build with an error thrown, exit status is 1.

  • yarn build with no error thrown, exit status is 0.

  • yarn react-static logs the usage info, exit status is 0, no error logged

I have not added tests yet, would love some, will look into it!

Motivation and Context

Closes/Fixes #1450

Types of changes

  • Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have updated the documentation accordingly
  • I have updated the CHANGELOG with a summary of my changes
  • My changes have tests around them

init() returns a Promise, and hence, would need to be awaited for, which is not possible top-level. Went for the `catch` as it's nicer than wrapping the try/catch in another async function, so we could await init().
@nddery
Copy link
Collaborator Author

nddery commented Jun 3, 2020

@SleeplessByte any idea on how we could add tests around bin/react-static, and if that is worth it (especially with RS8 changing things up) ?

@SleeplessByte
Copy link
Member

It's possible to call the bin/xxx programmatically. Might be worth it. I wouldn't do it for now; because RS7 is pretty complex :(

@nddery
Copy link
Collaborator Author

nddery commented Jun 4, 2020

Yeah that make sense. I guess I was mostly wondering of ways to provide the executable with the mocks and whatnot (templates). I guess that's the same conclusion than you've arrived at in #1383 (comment).

@SleeplessByte
Copy link
Member

(You got merge permissions btw)

@nddery nddery merged commit 87ea293 into react-static:master Jun 5, 2020
@nddery nddery deleted the fix/non-zero-exit branch June 5, 2020 14:01
@nddery
Copy link
Collaborator Author

nddery commented Jun 5, 2020

Haha I do! Do you feel like this warrant a 7.4.1 release ?

@SleeplessByte
Copy link
Member

If you need it, sure. But I think you also run a local copy right?

@nddery
Copy link
Collaborator Author

nddery commented Jun 8, 2020

I run a local copy for development but we do run the one from NPM when building during deploys. I can take care of making a release PR if you'd like (unclear to me exactly what the steps are), and then there would only be the tags to push to NPM or something ?

@nddery
Copy link
Collaborator Author

nddery commented Jun 29, 2020

@SleeplessByte any thoughts on this ? I don't think I can push a release myself but probably can make a PR for it (although, I'm not sure of what command to run for this).

@SleeplessByte
Copy link
Member

I forgot about this! You can create the release PR if you'd like, I'll merge and deploy then. I'll document which commands I run to release at some point.

85 hour work weeks coming to an end in about 1 month, then I can focus on getting some sponsorships for React Static and build v8.

@nddery
Copy link
Collaborator Author

nddery commented Jun 30, 2020

Thats a lot of work hours -- take care of yourself too! I'll open a PR for a release.

@nddery nddery mentioned this pull request Jun 30, 2020
8 tasks
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.

[Bug] Builds exit with a zero status code when errors are thrown
2 participants