-
-
Notifications
You must be signed in to change notification settings - Fork 597
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: close compiler, own sh script and output clearing
- Loading branch information
1 parent
2ed8c60
commit 6ded275
Showing
5 changed files
with
645 additions
and
674 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
6ded275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evenstensberg
I wonder why the change from
console.log
toconsole.error
in this commit?This change breaks the tests for us and while we can modify the tests, we now have no way to distinguish a real error from a log entry.
Conceptually I can't work out the sense in using
console.error
for what is clearly not an error.Concretely, we have this code:
Followed by:
6ded275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. We did it because when you used
--json
to pipe your arguments to, it would include the console log. @jhnns has some resources on why console.error is the best case here6ded275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. So in the case of:
webpack --json > stats.json
Why not disable the
console.log()
?I mean, isn't
--json
a way to request JSON output rather thanconsole.log()
?I also wonder how you got around this in tests? Are you not testing cases where there was an error? Say a bad webpack config?
6ded275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be an option, but in watch, watch will either way output that we’re watching the files, which I want to have there. We changed the tests
6ded275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this a done deal?
Asking so to know whether to go ahead and update our tests.
6ded275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes