Skip to content
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

Use JSON schema to validate webpack config (fixes #2971) #2974

Merged
merged 20 commits into from Sep 18, 2016

Conversation

Projects
10 participants
@gajus
Copy link
Member

gajus commented Sep 8, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Feature

What is the current behavior? (You can also link to an open issue here)

Validation is limited to a simple condition:

if(!options.entry && !options.plugins) {
  throw new Error("Passed 'options' object does not look like a valid webpack configuration");
}

What is the new behavior?

Validation logic is expressed using ./schemas/webpackOptionsSchema.json JSON schema.

Does this PR introduce a breaking change?

  • Yes

If this PR contains a breaking change, please describe the following...

  • Impact:

    An error is thrown when an unknown configuration is encountered in webpack options object.
    This error might appear now because previously unknown configuration was ignored.

  • Migration path for existing applications:

    Remove unknown configuration properties from webpack options object.
    To configure a loader, use LoaderOptionsPlugin.
    For a list of valid configuration properties, refer to the webpack documentation (https://webpack.github.io/docs/configuration.html).

  • Github Issue(s) this is regarding:

    #2971

Other information:

  • Need to discuss how to approach testing.

    JSON schema is declarative. Therefore, the contents of the schema should not be tested. The implementation testing is left to https://github.com/epoberezkin/ajv.

  • Need to discuss how to inform user about errors.

    Just throwing an error is not enough.

    Passed 'options' object does not look like a valid webpack configuration

    I am fine with the current implementation – throwing an error and printing detail info to the console.log. I can see how someone can dislike this.

Outstanding tasks

@gajus gajus changed the title Use JSON schema to validate webpack config Use JSON schema to validate webpack config (fixes #2971) Sep 8, 2016

},
"type": "object"
},
"output": {

This comment has been minimized.

@TheLarkInn

TheLarkInn Sep 8, 2016

Member

Missing output.auxiliaryComment, heres the PR for it.

I hadn't gotten to documenting it yet. 👀

#2113

It is essentially:
auxiliaryComment?: string|AuxCommentObject

If object properties are:

interface AuxCommentObject {
  root?: string
  commonjs?: string
  commonjs2?: string
  amd?: string
}

Let me know if this is unclear.

This comment has been minimized.

@gajus

@gajus gajus force-pushed the gajus:feature/use-json-schema-to-validate-webpack-config branch 2 times, most recently from 3339271 to f94e945 Sep 8, 2016

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 8, 2016

Tests are failing because:

[ { keyword: 'additionalProperties',
    dataPath: '',
    schemaPath: '#/additionalProperties',
    params: { additionalProperty: 'name' },
    message: 'should NOT have additional properties' } ]
[ { keyword: 'additionalProperties',
    dataPath: '',
    schemaPath: '#/additionalProperties',
    params: { additionalProperty: 'optimize' },
    message: 'should NOT have additional properties' } ]
[ { keyword: 'additionalProperties',
    dataPath: '',
    schemaPath: '#/additionalProperties',
    params: { additionalProperty: 'worker' },
    message: 'should NOT have additional properties' } ]
[ { keyword: 'additionalProperties',
    dataPath: '',
    schemaPath: '#/additionalProperties',
    params: { additionalProperty: 'updateIndex' },
    message: 'should NOT have additional properties' } ]
[ { keyword: 'additionalProperties',
    dataPath: '',
    schemaPath: '#/additionalProperties',
    params: { additionalProperty: 'stats' },
    message: 'should NOT have additional properties' } ]

can anyone contribute a description for these? (they are not present in https://webpack.github.io/docs/configuration.html)

if(!options.entry && !options.plugins) {
var webpackOptionsValidationErrors = validateWebpackOptions(options);
if(webpackOptionsValidationErrors.length) {
console.log("'options' validation errors:", webpackOptionsValidationErrors);

This comment has been minimized.

@sokra

sokra Sep 8, 2016

Member

You are only allowed use console.log in the CLI not in the API.

Instead rewrite the code to throw a CustomError with a validationErrors property. Add code to the CLI to check for this property and format the errors.

This comment has been minimized.

@gajus

gajus Sep 9, 2016

Author Member

What about users that use webpack programmatically? e.g. most of the React applications will have a setup using webpack and webpack-dev-middleware.

Do I need to document presence of the validationErrors property?

This comment has been minimized.

@sokra
@sokra

This comment has been minimized.

Copy link
Member

sokra commented Sep 8, 2016

Please add *.json to the .editorconfig.

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Sep 8, 2016

name is a valid property used to give configs a name when using multiple configurations in an array.

optimize is invalid. It's old syntax and no longer used.

worker is used by the worker-loader, but it should not be on the config object, but the config should use the LoadersOptionsPlugin instead.

stats is valid. It allows to pass stat options to the CLI.

updateIndex is used by the HMR test cases, but can also be rewritten to use the LoadersOptionsPlugin

@TheLarkInn

This comment has been minimized.

Copy link
Member

TheLarkInn commented Sep 8, 2016

Info on stats @gajus can be gathered from here.

https://github.com/webpack/webpack/blob/master/lib/Stats.js

@TheLarkInn

This comment has been minimized.

Copy link
Member

TheLarkInn commented Sep 8, 2016

Also would this error for a user if they added an additional custom property (some sort of metadata) to their config? I know quite a few who do this, and use those options, etc.

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Sep 8, 2016

I'm not sure if we should throw when additional keys are encountered... webpack 2 should not have too many breaking changes. It's ok to discourage it and display warnings, but I don't think that we should break. The sass- and the less-loader for instance provide the possibility to read options from the config (because we had to deal with functions as options).

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

Please add *.json to the .editorconfig.

b4e6604

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

worker is used by the worker-loader, but it should not be on the config object, but the config should use the LoadersOptionsPlugin instead.

In the case of /examples/web-worker/webpack.config.js, this ought to be a simple change from:

module.exports = {
    worker: {
        output: {
            filename: "hash.worker.js",
            chunkFilename: "[id].hash.worker.js"
        }
    }
}

to:

var webpack = require("../../");
module.exports = {
    plugins: [
        new webpack.LoaderOptionsPlugin({
            options: {
                worker: {
                    output: {
                        filename: "hash.worker.js",
                        chunkFilename: "[id].hash.worker.js"
                    }
                }
            }
        })
    ]
}

Correct?

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

Correct?

Yes.

I forgot npm link webpack.

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

@sokra

git clone https://github.com/webpack/webpack.git
cd webpack
npm install
npm link
npm link webpack
npm test

is giving:

  1) ConfigTestCases extract-text issue-14 should compile:
     Error: global leak detected: addresses
      at Runner.checkGlobals (/dev/node_modules/mocha/lib/runner.js:210:21)
      at Runner.<anonymous> (/dev/node_modules/mocha/lib/runner.js:73:10)
      at emitOne (events.js:101:20)
      at Runner.emit (events.js:188:7)
      at /dev/node_modules/mocha/lib/runner.js:557:14
      at done (/dev/node_modules/mocha/lib/runnable.js:287:5)
      at /dev/node_modules/mocha/lib/runnable.js:360:7
      at _combinedTickCallback (internal/process/next_tick.js:67:7)
      at process._tickCallback (internal/process/next_tick.js:98:9)

Unrelated to this PR.

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

worker is used by the worker-loader, but it should not be on the config object, but the config should use the LoadersOptionsPlugin instead.

70e4a1e

updateIndex is used by the HMR test cases, but can also be rewritten to use the LoadersOptionsPlugin

e42ddb2

@@ -1,5 +1,5 @@
root = true

[*.js]
[*.{js,json}]

This comment has been minimized.

@sokra

sokra Sep 9, 2016

Member

for npm reasons package.json must be 2 spaces indent

This comment has been minimized.

@gajus

gajus Sep 9, 2016

Author Member

1d4869a

Technically, we could have a separate rule for ./package.json and the rest.

throw new Error("Passed 'options' object does not look like a valid webpack configuration");
var webpackOptionsValidationErrors = validateWebpackOptions(options);
if(webpackOptionsValidationErrors.length) {
var validationError = new Error("Passed 'options' object does not look like a valid webpack configuration");

This comment has been minimized.

@sokra

sokra Sep 9, 2016

Member

Create a custom Error class and pass the validationErrors as argument, which is then added as property by the constructor.

example: https://github.com/webpack/webpack/blob/master/lib/CaseSensitiveModulesWarning.js

if (webpackOptionsValidationErrors.length) {
console.error("Configuration is invalid. Fix the following errors:", webpackOptionsValidationErrors);
process.exit(-1); // eslint-disable-line
}

This comment has been minimized.

@sokra

sokra Sep 9, 2016

Member

remove this block and catch the exceptions thrown by the API in bin/webpack.js instead.

This comment has been minimized.

@gajus

gajus Sep 9, 2016

Author Member

Have you seen the comment about this?

efc4113

Please advise if to proceed in your direction after you have read it.

gajus added a commit to gajus/webpack that referenced this pull request Sep 9, 2016

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Sep 9, 2016

No comments on the error handling?

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

The sass- and the less-loader for instance provide the possibility to read options from the config (because we had to deal with functions as options).

My understanding is that all loader-specific options need to be passed using webpack.LoaderOptionsPlugin.

It does not require a change to the loaders. It is only a configuration change.

@gajus gajus force-pushed the gajus:feature/use-json-schema-to-validate-webpack-config branch from 1d4869a to bae2488 Sep 9, 2016

gajus added a commit to gajus/webpack that referenced this pull request Sep 9, 2016

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Sep 9, 2016

There will still be loader queries in webpack 2 (and tbh I don't know what the difference between an option and a query is).

My problem with this change is, that it needs some time to be implemented in the community. Loader authors need to update their code and publish new versions of it, people need to update their dependencies. I would like to have a smooth transition where you get a warning with webpack 2 and an error with webpack 3.

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 9, 2016

Options and queries are interchangeable.

Loader authors are not affected. No code change is required to the loader to accept options passed using LoaderOptionsPlugin. Therefore, no update is needed of dependencies.

As I said earlier, this only affects webpack configuration.

On Sep 9, 2016, at 12:15, Johannes Ewald notifications@github.com wrote:

There will still be loader queries in webpack 2 (and tbh I don't know what the difference between an option and a query is).

My problem with this change is, that it needs some time to be implemented in the community. Loader authors need to update their code and publish new versions of it, people need to update their dependencies. I would like to have a smooth transition where you get a warning with webpack 2 and an error with webpack 3.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

gajus added some commits Sep 9, 2016

feature: log options validations errors in CLI
Ajv is using doT to compile validation logic. There is minimal overhead validating an object against a pre-compiled schema. In case of the CLI, options will be validated once by the CLI engine and the second time by API.

I prefer to explicitly handle error checking rather than relying on an error getting thrown by the API engine.

API needs to retain “validationErrors” object nevertheless to enable debugging when using webpack programmatically.

@gajus gajus force-pushed the gajus:feature/use-json-schema-to-validate-webpack-config branch from 76920b3 to 4526824 Sep 17, 2016

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 17, 2016

Message phrasing has been improved. 9e0a95e

Can this be merged?

@sokra sokra merged commit 9e0a95e into webpack:master Sep 18, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 11.429%
Details
@rupesh1

This comment has been minimized.

Copy link

rupesh1 commented Sep 19, 2016

If I do an npm install with a package.json setup like: "webpack": "https://github.com/webpack/webpack"

I get the error:

> cross-env NODE_ENV=production webpack --bail --progress --color --display-error-details --profile

module.js:327
    throw err;
    ^

Error: Cannot find module '../schemas/webpackOptionsSchema.json'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/Users/rupesh/Documents/src/project-name/node_modules/webpack/lib/validateWebpackOptions.js:5:28)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)

And sure enough the folder and file in the path mentioned are missing on the file system.
Is anyone else seeing this?

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Sep 19, 2016

schemas is missing in package.json files. You can send a PR.

@gajus

This comment has been minimized.

Copy link
Member Author

gajus commented Sep 19, 2016

schemas is missing in package.json files. You can send a PR.

#3013

@MoOx

This comment has been minimized.

Copy link
Contributor

MoOx commented Sep 20, 2016

A breaking change from 2.1.0-beta22 to 2.1.0-beta23 really sucks. You might really broke a lot of CI system today...

@thattomperson

This comment has been minimized.

Copy link

thattomperson commented Sep 20, 2016

beta22 and beta23 are both prerelease versions and there has not been a 2.x release yet.
sokra has every right as defined by semver to make breaking changes

@MoOx

This comment has been minimized.

Copy link
Contributor

MoOx commented Sep 20, 2016

Having the "right" to do something is a thing. Not caring about current users (that help to test prerelease right?) and upgrade path is another thing.

@thattomperson

This comment has been minimized.

Copy link

thattomperson commented Sep 20, 2016

"testing" and "depending" on prerelease software are different things
if webpack can't make breaking changes then it will just end up with tonnes of bloat and be slow

I think this is a good change, random plugins should not pollute the config with random keys in case webpack wants to use them in the future

instead of complaining about it, we could help come up with a solution, for example,
maybe webpack could add a loaderConfig: {} option which loader and plugin authors could use for their config without using the global config

so loaders just go config.loaderConfig[loaderName] instead of config[loaderName]
or as sokra mentioned earlier

allowing any value in queries

im guessing that would include objects and functions

Also thanks, I have been looking into static site generators atm so I might look at phenomic when I get home

@MoOx

This comment has been minimized.

Copy link
Contributor

MoOx commented Sep 20, 2016

I think this is a good change, random plugins should not pollute the config with random keys in case webpack wants to use them in the future

It definitely is.

I think a simpler workaround than having to use a plugin to pass data should be offered. The errow thrown does not event mention a solution. It could say

If your configuration currently use non webpack related properties (eg: eslint, sass, postcss etc.), you should pass those data by using `LoaderOptionsPlugin`. Please refer to XXX for more detail

I think the best solution will be to support real JS for query. It's the best user experience we can offer an make configuration for loader even more clear than having to split in multiple places.

rupesh1 added a commit to rupesh1/hapi-webpack-dev-middleware that referenced this pull request Sep 21, 2016

@huizi666

This comment has been minimized.

Copy link

huizi666 commented Mar 1, 2017

i have a trouble in webpack2 . but i'm tring to use webpack1' way to code my webapck2.
and ....there is some thing wrong in postcss-loader
<addr> module : {
loaders : [
{test:/.js$/,loader:'babel-loader',exclude:path.resolve(__dirname,'node_modules'),
//绝对路径 只解析src下的提高打包性能
include:path.resolve(__dirname,'src')},
{test:/.css$/,loader:'style-loader!css-loader!postcss-loader'}
]
},
plugins : {
//for postcss
new webpackLoaderOptionsPlugin({
options : {
postcss : function(){
return [auroprefixer]
},
devServer: {
contentBase: "./public", //本地服务器所加载的页面所在的目录
colors: true, //终端中输出结果为彩色
historyApiFallback: true, //不跳转
inline: true //实时刷新
}
}
}),
======just part of them

there is error
angular-oms-front\webpack.config.js:27
new webpackLoaderOptionsPlugin({
^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: Unexpected identifier
at Object.exports.runInThisContext (vm.js:76:16)
at Module._compile (module.js:542:28)
at Object.Module._extensions..js (module.js:579:10)
at Module.load (module.js:487:32)
at tryModuleLoad (module.js:446:12)
at Function.Module._load (module.js:438:3)
at Module.require (module.js:497:17)
at require (internal/module.js:20:19)
at requireConfig (E:\another\angular-oms-front\node_modules\webpack\bin\convert-argv.js:96:18)
at E:\another\angular-oms-front\node_modules\webpack\bin\convert-argv.js:109:17

(i'm not good at english .waiting your reply)

@bebraw

This comment has been minimized.

Copy link
Member

bebraw commented Mar 1, 2017

@huizi666 plugins should be an array. Could you please open support request at Stack Overflow and other channels? Thanks. 👍

@huizi666

This comment has been minimized.

Copy link

huizi666 commented Mar 2, 2017

I dont understand about "support request",but this is my git ,could you please gitclone git@github.com:huizi666/angular-webpack.git . And checkout the webpack.config.js Thanks~~ @bebraw

@huizi666

This comment has been minimized.

Copy link

huizi666 commented Mar 2, 2017

@bebraw may you can send me an email : superhuizi0716@gmail.com ~ ^_^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.