Moved failed build logic from webpack into respective callers. #404
Conversation
Codecov Report
@@ Coverage Diff @@
## master #404 +/- ##
=========================================
- Coverage 99.3% 99.3% -0.01%
=========================================
Files 73 73
Lines 1873 1869 -4
Branches 294 293 -1
=========================================
- Hits 1860 1856 -4
Misses 13 13
Continue to review full report at Codecov.
|
Blackbaud-BobbyEarl
requested review from
Blackbaud-SteveBrush and
Blackbaud-PaulCrowder
cli/build.js
Outdated
| .catch(err => { | ||
|
|
||
| // Conditionally calling process.exit so it can bubble up if being called from e2e | ||
| if (!imported) { |
There was a problem hiding this comment.
@Blackbaud-BobbyEarl Most of the changes look great. I'm confused what "imported" means in this context?
There was a problem hiding this comment.
Valid question. I use that so that I could conditionally handle logging errors / calling process.exit.
When skyux build command is called that imported param is falsy. That's b/c we want the errors shown and, more importantly, the process to exit and return a non-zero exit code.
When running skyux e2e, that file will require('./build'), and pass in true for that parameter as seen at https://github.com/blackbaud/skyux-builder/pull/404/files#diff-27e8eb2b645e7d3144b02020922373aaR140. This allows the e2e command to not only log the errors, but most importantly, lets it's catch handler run. If not, the e2e wouldn't have an opportunity to perform cleanup when something in build fails.
There was a problem hiding this comment.
That makes sense.
I'm curious if both build and e2e should consume the same "bundler", so that we wouldn't have to deal with yet another function argument that's specific only to e2e. The bundler could return a promise that's either resolved or rejected.
Would build's linting errors also get rejected properly during e2e?
https://github.com/blackbaud/skyux-builder/blob/master/cli/build.js#L179
There was a problem hiding this comment.
I like that suggestion, and good catch about my current implementation not handling the linting errors. Let me see what I can work up. May do some work w/o updating the unit tests first.
There was a problem hiding this comment.
Cool! No worries if it's a major refactor
|
Took some refactoring, but I liked your suggestion @Blackbaud-SteveBrush. This is ready for another look. |
…baud#404) * Moved failed build logic from webpack into respective callers (needed for e2e). * Fixed linting error * Fixed errors. Making logs more quiet. * Refactored build into run-build. Having cli/build + cli/e2e call it. * Fixed linting errors. * Converting build stats to chunks for e2e
blackbaud/skyux2#948