Avoid swallowing JSON5 error output #34
Conversation
|
|
src/index.js
Outdated
| value = JSON5.parse(source); | ||
| } catch (e) { | ||
| throw new Error('Error parsing JSON5', (e)); | ||
| } |
There was a problem hiding this comment.
This was not doing anything except swallowing JSON5's error info. An error is thrown either way, so might as well do nothing and get the useful info.
There was a problem hiding this comment.
We need change throw Error on https://webpack.js.org/api/loaders/#thisemiterror
There was a problem hiding this comment.
Do you mean switch it from throwing entirely (even though it was not emitting before)?
There was a problem hiding this comment.
Before loader swallow error, with this.emitError it will work as expected. Just:
try {
value = JSON5.parse(source);
} catch (e) {
this.emitError(e);
}
There was a problem hiding this comment.
Gotcha. Will do when I get home from work.
|
I signed the CLA, so I dunno why it says Travis is failing because it's configuration is wrong/outdated:
|
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
=======================================
- Coverage 83.33% 75% -8.34%
=======================================
Files 2 2
Lines 6 4 -2
=======================================
- Hits 5 3 -2
Misses 1 1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
=======================================
Coverage 83.33% 83.33%
=======================================
Files 2 2
Lines 6 6
=======================================
Hits 5 5
Misses 1 1
Continue to review full report at Codecov.
|
src/index.js
Outdated
| value = JSON5.parse(source); | ||
| } catch (e) { | ||
| throw new Error('Error parsing JSON5', (e)); | ||
| } |
There was a problem hiding this comment.
We need change throw Error on https://webpack.js.org/api/loaders/#thisemiterror
|
Don't see on CI |
|
Weird. Travis was failing with that error before you closed and re-opened the PR. |
JakobJingleheimer
changed the title
|
Please accept CLA, code looks good! |
|
@evilebottnawi I already did (I mentioned above). I got an email confirmation too. |
|
Looks commit email is not same as you send in CLA |
|
Correct. My github privacy settings do not expose my actual email address. https://help.github.com/en/articles/about-commit-email-addresses Your CLA bot does not support providing the correct one. |
|
Very strange, can you check this again? I can't merge without |
|
Yay! It worked this time :) |
|
@jshado1 Thanks, i will update some deps and fix CI in near PR and release new version |
Resolves #33