-
-
Notifications
You must be signed in to change notification settings - Fork 597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] configuration validation #240
[feature] configuration validation #240
Conversation
I believe the failing tests here are addressed in #238 |
Yeah exactly, these are tests due to the new version of |
Let's wait for the fix on that to land and then do a review/rebase |
bin/convert-argv.js
Outdated
@@ -3,6 +3,10 @@ var fs = require("fs"); | |||
fs.existsSync = fs.existsSync || path.existsSync; | |||
var interpret = require("interpret"); | |||
var prepareOptions = require("./prepareOptions"); | |||
var webpackConfigurationSchema = require("../schemas/webpackConfigurationSchema.json"); | |||
var validateSchema = require("webpack/lib/validateSchema"); | |||
var WebpackOptionsValidationError = require("webpack/lib/WebpackOptionsValidationError"); |
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.
Please use webpack.WebpackOptionsValidationError
instead
bin/convert-argv.js
Outdated
@@ -3,6 +3,10 @@ var fs = require("fs"); | |||
fs.existsSync = fs.existsSync || path.existsSync; | |||
var interpret = require("interpret"); | |||
var prepareOptions = require("./prepareOptions"); | |||
var webpackConfigurationSchema = require("../schemas/webpackConfigurationSchema.json"); | |||
var validateSchema = require("webpack/lib/validateSchema"); |
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.
Please use webpack.validateSchema
instead
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.
Can you switch this over @jbottigliero?
@jbottigliero mind rebasing against the master branch? |
5551699
to
9af6795
Compare
@ev1stensberg – rebased and updated. I wasn't able to reproduce the test failure in @ooflorent – for the usage of |
@jbottigliero sure, happens sometimes, let's do another try |
Re-started your build, let's cross fingers 🤞 |
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
Updated based on #229. cc. @TheLarkInn |
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.
LGTM, passing to @TheLarkInn or @fokusferit to 2nd review
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.
Well, as far as I can see (read also the source code of validateSchema and WebpackOptionsValidationError, it looks fine.
I'm currently thinking of more test cases, I'd like to to throw in some more , maybe complex configurations to see if everything is still fine. @ev1stensberg any thoughts on that ? Otherwise I would approve it.
Thanks for taking a look @ev1stensberg and @fokusferit. After taking another look at the code in When When you're using a Seems like I could/should also add a case for the |
Hey @jbottigliero what do you think regarding about adding a test case for |
@jbottigliero Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ev1stensberg Please review the new changes. |
@fokusferit, @ev1stensberg, just took a look at this... Unless I'm mistaking there are few things that need to happen in order to make this work (at least to validate the It appears the
I think ideally we get For now, I've added two new tests and resolved merge conflicts. |
(tests are passing now because it's still just |
Support for Once released, I'll open a PR bumping |
- updates to `ajv@6.1.0` - updates to `ajv-keywords@3.1.0` (required for _better_ validation in webpack/webpack-cli#240)
Maybe we can keep it like this an open an issue up for discussion? Instead of rushing it |
@ev1stensberg works for me. I did just open webpack/webpack#6430... The dependency version bump in core does feel pretty heavy-handed based on the original intent of this PR. I'll be watching for feedback, thanks again. |
Can you make an issue at webpack-cli with what we've just discussed and link to the 6430 and this pr? Thanks! |
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.
I don't see any specific error message here except that you're throwing the validationSchema. Could you add an extra log on top of the validation schema that's something like: You passed X into webpack, we expect Y
?
The console.error will log:
I can certainly add something along the lines of |
Previous looked fine, but some extra indication would be nice, like the schema, but:
|
But that might be a schema thing, so LGTM |
console.error( | ||
"Config did not export an object or a function returning an object." | ||
error.message, | ||
`\nReceived: ${typeof options} : ${JSON.stringify(options, null, 2)}` |
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.
Check the json stringify callback to b correct @fokusferit @ematipico
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.
I would add a newline \n
straight after :
. But the rest looks good to me
I really like the correct indentation of the configuration error, it makes more cleaner and easier to read! |
Thanks ☝️💙💙 |
webpack/lib/validateSchema
What kind of change does this PR introduce?
Feature
Did you add tests for your changes?
Yes – I'll probably add more based on actual desired implementation.
If relevant, did you update the documentation?
N/A – for now.
Summary
This is an port of webpack/webpack#6255 which is intended to resolve webpack/webpack#4977.
Does this PR introduce a breaking change?
Other information
Some outstanding questions (from webpack/webpack#6255):
With the code now being in separate repositories/modules, I could see how the proposed solution is less than ideal. I'm up for discussing how to move forward!