Fix early exit when enableBuild is true #61
Conversation
Codecov Report
@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 30.03% 34.41% +4.37%
==========================================
Files 13 13
Lines 576 587 +11
Branches 118 114 -4
==========================================
+ Hits 173 202 +29
+ Misses 403 384 -19
- Partials 0 1 +1
Continue to review full report at Codecov.
|
|
It's better to throw an error instead of silently killing the process. This is what it would look like with this MR: |
| process.exit(exitCode) | ||
| } | ||
| ) | ||
| const exitCodes = await Promise.all(checkers.map((checker) => spawnChecker(checker, userConfig, localEnv))); |
There was a problem hiding this comment.
if we await here. The checker and the rollup build will run sequentially. Is it better to run parallelly? 🤔
There was a problem hiding this comment.
Depends on what the purpose of running the checker on build is? I can see both being valid perhaps? Even if you wanted to run it in parallel, you could still utilize async/await (just spawn a new async function at the end). But yeah, I guess the current behavior should perhaps remain for now.
There was a problem hiding this comment.
Maybe we can add an option in the future to set whether run check sequentially. We can keep the parallel behaviour for now. 😉
There was a problem hiding this comment.
Sounds good. I can make the changes to this branch tomorrow, unless you want to make them yourself sooner than that, up to you.
There was a problem hiding this comment.
Sounds good. I can make the changes to this branch tomorrow, unless you want to make them yourself sooner than that, up to you.
There was a problem hiding this comment.
Sounds good, thanks for the write-up! Very odd that the build is failing though, it's not even on a line that I touched 🤔
There was a problem hiding this comment.
oooh it was because of a missing semicolon. I can strongly recommend you to add some stricter linting to this project. Semi-colons should be mandatory on all the lines 😅
There was a problem hiding this comment.
I'd suggest you to take a look at https://www.npmjs.com/package/eslint-config-airbnb-base. But either way, this MR should be good now.
There was a problem hiding this comment.
Yeah, this PR can be merged now. I'll do some more work and then release a version. (still haven't use semantic-release now 😅)
It's because in this line
})
// spawn an async runner that we don't wait for in order to avoid blocking the build from continuing in parallel
(async () => {There should be a semicolon before (async because it's a new line that starts with ). Non-semi is also acceptable (such as Vite). I'll do some style clean in the following commits and release a new version.
Thanks for your PR! 🤗
Fixes #60.