add rulesdir option #7
Conversation
…(friendly-errors-webpack-plugin)
|
Dont't understand why the tests are failing :/ They are green on my machine |
index.js
Outdated
| /* eslint-disable import/no-dynamic-require */ // --> OFF | ||
| filepath = path.resolve(filepath); | ||
| try { | ||
| var module = require(filepath); |
There was a problem hiding this comment.
Please change this line to const module = require(filepath); // eslint-disable-line import/no-dynamic-require. I also would use a different variable instead of module to prevent shadowing the global module variable. Maybe rule instead?
index.js
Outdated
| function lint(source, options, webpack, done) { | ||
| try { | ||
| if (options.customRules) { | ||
| options.customRules.forEach(rule => HTMLHint.addRule(rule)); | ||
| } | ||
| if (options.rulesdir) { |
There was a problem hiding this comment.
Can you camelcase this option to rulesDir instead?
index.js
Outdated
| @@ -80,7 +111,7 @@ function lint(source, options, webpack, done) { | |||
| emitter = webpack.emitWarning; | |||
| } | |||
|
|
|||
| emitter(options.formatter(report)); | |||
| emitter(new Error(options.formatter(report))); | |||
There was a problem hiding this comment.
I believe the tests are failing because this is now being emitted as an error. This shouldn't be the case, especially if emitting as a warning not an error.
There was a problem hiding this comment.
On my dev machine all the tests are green. And I added this because of a warning in friendly-errors-webpack-plugin : "value string instead of error instance".
I do not understand why they are failing on Travis :/
There was a problem hiding this comment.
aha, I realised what was going wrong, travis + my local dev envrionment uses the yarn.lock which has an older version of webpack in the dependencies. It seems like webpack introduced a breaking change between 2.2.1 and 2.4.1 that throws if you emit non errors. If you sync your branch with master the tests should start passing 😄
|
Thanks for the review. Il will try to do the changes tomorrow. |
|
I made the changes. |
index.js
Outdated
| const ruleOption = options[ruleObj.id]; // we can pass a value to the rule | ||
| ruleObj.rule(HTMLHint, ruleOption); | ||
| } catch (err) { | ||
| throw new Error(err.message); |
There was a problem hiding this comment.
Is this try catch necessary? By wrapping it like this you'll lose the stack trace etc
package.json
Outdated
| @@ -39,6 +40,7 @@ | |||
| "devDependencies": { | |||
| "chai": "^3.5.0", | |||
| "codecov-lite": "^0.1.3", | |||
| "glob": "^7.1.1", | |||
| "mocha": "^3.0.2", | |||
There was a problem hiding this comment.
This should be a dependency, not a dev dependency (also make sure the yarn.lock file is updated)
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
==========================================
+ Coverage 88.05% 89.15% +1.09%
==========================================
Files 1 1
Lines 67 83 +16
==========================================
+ Hits 59 74 +15
- Misses 8 9 +1
Continue to review full report at Codecov.
|
|
Released as 1.2.0, thanks for the contribution! 💯 |
|
Great :) |
Hi,
We are curently using VueJs and to be able to have your loader working we had to use the rulesdir feature of HtmlHint.
We think this could be useful for anyone.
The code is inspired by HtmlHint itself : https://github.com/yaniswang/HTMLHint/blob/152a114f277645474ca1f70d68a0e01e75f3eb51/bin/htmlhint#L142 .
What do you think ?