js-dxtools / webpack-validator

Validates your webpack config with Joi
MIT License
295 stars 29 forks source link

Add "rules" concept and `resolve.root` linting #81

Closed jonathanglasmeyer closed 8 years ago

jonathanglasmeyer commented 8 years ago

Hey guys, here comes something. :) I tried out Joi 9's .extend feature which allows us to write custom validations and have them appear in the some formatted stdout that Joi provides + lets us use the succint, declarative style for more advanced features. This PR does multiple things (I know i know :) - i totally spike coded this):

  1. Introduce the concept of "rules" that can be opted in or out to, analogous to eslint rules.
  2. Implements a basic validation for #79 -- it checks that items in paths specified via resolve.root do not nameclash with node_module packages
  3. Implements actual checking of the existence of paths (via src/types/absolutePaths now using Joi.extend and thereby having the power to do file system checks)
  4. Introduces a breaking change discussed in #73, so that we only have two arguments to validate(), first: the webpack config object, second: an options object for webpack-validator.

    Comment on point 1: in order to implement "schema configurability" (toggling rules on and off), the schema is now wrapped in a function that takes a schemaOptions object; it will pass this to children. The bulk of changes pertains to a necessary internal interface change for allValid: it is now more similar to allInvalid, also taking the input like { input: <the-test-config-slice> }. This allows overwriting the sub-schema used in order to test different schemaOptions, i.e write tests for a config slice with a relevant rule disabled and enabled (example can be seen in src/properties/resolve/index.test.js, valid #12 -- we override the schema in order to test that disabling the rule "no-root-files-node-modules-nameclash" will allow this resolve.root configuration, even if the specified folders contain files/folders that match a node_modules dependency name.

I'm pretty happy with this and look forward to your feedback! Edit: cc @bebraw @kentcdodds @nyrosmith :)

bebraw commented 8 years ago

Looks good to me. I added a few comments inline.

codecov-io commented 8 years ago

Current coverage is 100%

Merging #81 into master will not change coverage

  1. 2 files (not in diff) in src/types were created. more
  2. 11 files (not in diff) in src/properties were created. more
  3. File ...in/script-parsers.js (not in diff) was deleted. more
@@           master   #81   diff @@
===================================
  Files           1    17    +16   
  Lines          14   107    +93   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits           14   107    +93   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last updated by f01b917...c8f1fa7

jonathanglasmeyer commented 8 years ago

From my side this is ready to merge, i'll wait some days for more feedback though as this is a breaking change that'll shape the future of this tool.

bebraw commented 8 years ago

No objections? I would love to see this go forward. 👍

jonathanglasmeyer commented 8 years ago

Seems like it. :) I'm on holidays till next Thursday, no laptop with me. The branch would have to be rebased on master, if you want to do that, you can merge it into master after that. 👍🏻