[feature] configuration validation #240

jbottigliero · 2018-01-08T02:15:31Z

uses JSON schema to validate configuration types.
- validated via webpack/lib/validateSchema
adds tests for various configuration types
- adds negative tests for an invalid type and array.

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?

No, it's not expected to... However, it may cause previously invalid configuration objects to throw errors not seen before.

Other information

Some outstanding questions (from webpack/webpack#6255):

Is using JSON schema validation too much for this?

It does seem like it might be better to combine this validation with the configuration options validation. This would be a much larger change and is worthy of a discussion.

Should it be using the WebpackOptionsValidationError class? The updated output still leaves a bit to be desired. This could be accomplished using the same class or it's own.

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!

jbottigliero · 2018-01-08T02:22:31Z

I believe the failing tests here are addressed in #238

ematipico · 2018-01-08T13:43:32Z

Yeah exactly, these are tests due to the new version of webpack and how it outputs the entry points.

evenstensberg · 2018-01-08T15:36:18Z

Let's wait for the fix on that to land and then do a review/rebase

ooflorent · 2018-01-08T16:29:23Z

bin/convert-argv.js

@@ -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");


Please use webpack.WebpackOptionsValidationError instead

ooflorent · 2018-01-08T16:29:46Z

bin/convert-argv.js

@@ -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");


Please use webpack.validateSchema instead

Can you switch this over @jbottigliero?

evenstensberg · 2018-01-08T18:08:49Z

@jbottigliero mind rebasing against the master branch?

jbottigliero · 2018-01-08T19:10:14Z

@ev1stensberg – rebased and updated. I wasn't able to reproduce the test failure in node@6 locally, should I just re-trigger the build? Any insight here would be helpful!

@ooflorent – for the usage of webpack.*... I was basing my implementation/usage of webpack lib files off of lib/commands/migration.js. I'm not super familiar with the build process and this package's association between webpack core, yet. Is it safe to require webpack core here? I'll dig through the code myself, but any advice in direction would be helpful!

evenstensberg · 2018-01-08T19:50:04Z

@jbottigliero sure, happens sometimes, let's do another try

evenstensberg · 2018-01-08T20:06:37Z

Re-started your build, let's cross fingers 🤞

webpack-bot · 2018-01-08T20:08:56Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

jbottigliero · 2018-01-09T01:34:43Z

@ooflorent – for the usage of webpack.*... I was basing my implementation/usage of webpack lib files off of lib/commands/migration.js. I'm not super familiar with the build process and this package's association between webpack core, yet. Is it safe to require webpack core here? I'll dig through the code myself, but any advice in direction would be helpful!

Updated based on #229.

cc. @TheLarkInn

evenstensberg

LGTM, passing to @TheLarkInn or @fokusferit to 2nd review

fokusferit

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.

jbottigliero · 2018-01-23T15:20:55Z

Thanks for taking a look @ev1stensberg and @fokusferit.

After taking another look at the code in bin/convert-argv.js, maybe the validation could be more strict?

When processConfiguredOptions is called, it looks like the configuration can only ever be a Promise, object or array of objects.

When you're using a function for a configuration, it's actually handled by prepareOptions.

Seems like I could/should also add a case for the Promise configuration.

fokusferit · 2018-01-31T14:16:39Z

Hey @jbottigliero what do you think regarding about adding a test case for Promiseconfiguration? Just wanted to follow up :)

webpack-bot · 2018-01-31T16:13:50Z

@jbottigliero Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ev1stensberg Please review the new changes.

jbottigliero · 2018-01-31T16:15:12Z

@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 Promise instance).

It appears the ajv-keywords package doesn't actually support "instanceof": "Promise". I plan to open a ticket there to discuss whether or not it could/should be added – I'll follow-up on this.

An alternative would be to add a custom validator similar towebpack's addition of absolutePath
... because we're sharing the validator, I'll need to open a PR against webpack core. EDIT: A better alternative would just be adding Promise as custom constructor... but would still need to happen in webpack core.

I think ideally we get Promise added to the ajv-keywords package.

For now, I've added two new tests and resolved merge conflicts.

jbottigliero · 2018-01-31T16:20:48Z

(tests are passing now because it's still just "instanceof": "Function")

jbottigliero · 2018-01-31T20:36:34Z

Support for "instanceof": "Promise" was just merged – ajv-validator/ajv-keywords#51 (closed ajv-validator/ajv-keywords#52).

Once released, I'll open a PR bumping webpack core's dependency version and update here.

- updates to `ajv@6.1.0` - updates to `ajv-keywords@3.1.0` (required for _better_ validation in webpack/webpack-cli#240)

evenstensberg · 2018-01-31T21:52:00Z

Maybe we can keep it like this an open an issue up for discussion? Instead of rushing it

jbottigliero · 2018-01-31T21:59:47Z

@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.

evenstensberg · 2018-01-31T22:54:40Z

Can you make an issue at webpack-cli with what we've just discussed and link to the 6430 and this pr? Thanks!

evenstensberg

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?

jbottigliero · 2018-01-31T23:16:28Z

The console.error will log:

Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.
 - configuration should be one of these:
   object | [object] | function
   Details:
    * configuration should be an object.
      -> A webpack configuration object.
    * configuration should be an array:
      [object]
      -> An array of webpack configuration objects.
    * configuration should be an instance of function
      -> A promise that resolves with a configuration object, or an array of configuration objects.

I can certainly add something along the lines of Received: ${typeof options} (${options}) to this.

evenstensberg · 2018-02-01T00:31:30Z

Previous looked fine, but some extra indication would be nice, like the schema, but:


Webpack has been initialised using a configuration object that does not match the API schema. Recieved: $type 
...

evenstensberg · 2018-02-01T00:32:12Z

But that might be a schema thing, so LGTM

evenstensberg · 2018-02-01T01:04:10Z

bin/convert-argv.js

 			console.error(
-				"Config did not export an object or a function returning an object."
+				error.message,
+				`\nReceived: ${typeof options} : ${JSON.stringify(options, null, 2)}`


Check the json stringify callback to b correct @fokusferit @ematipico

I would add a newline \n straight after :. But the rest looks good to me

ematipico · 2018-02-01T09:18:43Z

I really like the correct indentation of the configuration error, it makes more cleaner and easier to read!

evenstensberg · 2018-02-02T06:43:07Z

Thanks ☝️💙💙

webpack-bot added PR: unreviewed PR: CI-not-ok labels Jan 8, 2018

jbottigliero mentioned this pull request Jan 8, 2018

Issue #4977 - Configuration Validation webpack/webpack#6255

Closed

ooflorent suggested changes Jan 8, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Jan 8, 2018

jbottigliero force-pushed the feature/configuration-validation branch from 5551699 to 9af6795 Compare January 8, 2018 18:43

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jan 8, 2018

jbottigliero closed this Jan 8, 2018

jbottigliero reopened this Jan 8, 2018

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jan 8, 2018

evenstensberg assigned TheLarkInn Jan 9, 2018

evenstensberg approved these changes Jan 22, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jan 22, 2018

EugeneHlushko mentioned this pull request Jan 23, 2018

Add info-verbosity into json schema #249

Closed

fokusferit reviewed Jan 23, 2018

View reviewed changes

jbottigliero mentioned this pull request Jan 31, 2018

Add Promise support to instanceof ajv-validator/ajv-keywords#52

Closed

jbottigliero pushed a commit to jbottigliero/webpack that referenced this pull request Jan 31, 2018

update ajv + ajv-keywords

61b75b7

- updates to `ajv@6.1.0` - updates to `ajv-keywords@3.1.0` (required for _better_ validation in webpack/webpack-cli#240)

jbottigliero mentioned this pull request Jan 31, 2018

update ajv + ajv-keywords webpack/webpack#6430

Merged

evenstensberg requested changes Jan 31, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Jan 31, 2018

adds 'Received' output on configuration validation error

c9250d8

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Jan 31, 2018

jbottigliero mentioned this pull request Jan 31, 2018

configuration validation : strict "Promise" instanceof validation #258

Closed

evenstensberg approved these changes Feb 1, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 1, 2018

evenstensberg requested review from ematipico and fokusferit February 1, 2018 00:32

evenstensberg reviewed Feb 1, 2018

View reviewed changes

ematipico approved these changes Feb 1, 2018

View reviewed changes

evenstensberg merged commit a15807b into webpack:master Feb 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] configuration validation #240

[feature] configuration validation #240

jbottigliero commented Jan 8, 2018

jbottigliero commented Jan 8, 2018

ematipico commented Jan 8, 2018

evenstensberg commented Jan 8, 2018

ooflorent Jan 8, 2018

ooflorent Jan 8, 2018

TheLarkInn Jan 8, 2018

evenstensberg commented Jan 8, 2018

jbottigliero commented Jan 8, 2018

evenstensberg commented Jan 8, 2018

evenstensberg commented Jan 8, 2018

webpack-bot commented Jan 8, 2018

jbottigliero commented Jan 9, 2018

evenstensberg left a comment

fokusferit left a comment

jbottigliero commented Jan 23, 2018

fokusferit commented Jan 31, 2018

webpack-bot commented Jan 31, 2018

jbottigliero commented Jan 31, 2018 •

edited

Loading

jbottigliero commented Jan 31, 2018

jbottigliero commented Jan 31, 2018 •

edited

Loading

evenstensberg commented Jan 31, 2018

jbottigliero commented Jan 31, 2018

evenstensberg commented Jan 31, 2018

evenstensberg left a comment

jbottigliero commented Jan 31, 2018

evenstensberg commented Feb 1, 2018

evenstensberg commented Feb 1, 2018

evenstensberg Feb 1, 2018

ematipico Feb 1, 2018

ematipico commented Feb 1, 2018

evenstensberg commented Feb 2, 2018

[feature] configuration validation #240

[feature] configuration validation #240

Conversation

jbottigliero commented Jan 8, 2018

jbottigliero commented Jan 8, 2018

ematipico commented Jan 8, 2018

evenstensberg commented Jan 8, 2018

ooflorent Jan 8, 2018

Choose a reason for hiding this comment

ooflorent Jan 8, 2018

Choose a reason for hiding this comment

TheLarkInn Jan 8, 2018

Choose a reason for hiding this comment

evenstensberg commented Jan 8, 2018

jbottigliero commented Jan 8, 2018

evenstensberg commented Jan 8, 2018

evenstensberg commented Jan 8, 2018

webpack-bot commented Jan 8, 2018

jbottigliero commented Jan 9, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

fokusferit left a comment

Choose a reason for hiding this comment

jbottigliero commented Jan 23, 2018

fokusferit commented Jan 31, 2018

webpack-bot commented Jan 31, 2018

jbottigliero commented Jan 31, 2018 • edited Loading

jbottigliero commented Jan 31, 2018

jbottigliero commented Jan 31, 2018 • edited Loading

evenstensberg commented Jan 31, 2018

jbottigliero commented Jan 31, 2018

evenstensberg commented Jan 31, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

jbottigliero commented Jan 31, 2018

evenstensberg commented Feb 1, 2018

evenstensberg commented Feb 1, 2018

evenstensberg Feb 1, 2018

Choose a reason for hiding this comment

ematipico Feb 1, 2018

Choose a reason for hiding this comment

ematipico commented Feb 1, 2018

evenstensberg commented Feb 2, 2018

jbottigliero commented Jan 31, 2018 •

edited

Loading

jbottigliero commented Jan 31, 2018 •

edited

Loading