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

Don't mutate stats configuration (webpack 3.8.0+ compatibility) #1174

Merged

Conversation

@BenoitZugmeyer
Copy link
Contributor

@BenoitZugmeyer BenoitZugmeyer commented Nov 3, 2017

What kind of change does this PR introduce?

Bugfix

Did you add or update the examples/?

No

Summary

The webpack configuration stats property has the same format as the devServer.stats property, so the same object is frequently reused to define the same stats output.

Since webpack v3.8.0, the stats property is validated more strictly by webpack (see webpack/webpack@e0d4501#diff-c6d9e330d5284ccaee0ea220907a2280 and webpack/webpack@5761875#diff-c6d9e330d5284ccaee0ea220907a2280). webpack-dev-server is mutating this stats object by adding a colors property, which is an object and doesn't match the webpack configuration schema, causing a fatal error:

Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema.
 - configuration.stats should be one of these:
   object { context?, hash?, version?, timings?, performance?, depth?, assets?, env?, colors?, maxModules?, chunks?, chunkModules?, modules?, children?, cached?, cachedAssets?, reasons?, source?, warnings?, errors?, warningsFilter?, excludeAssets?, excludeModules?, exclude?, entrypoints?, errorDetails?, chunkOrigins?, modulesSort?, moduleTrace?, chunksSort?, assetsSort?, publicPath?, providedExports?, usedExports?, optimizationBailout? } | boolean | "none" | "errors-only" | "minimal" | "normal" | "detailed" | "verbose"
   Used by the webpack CLI program to pass stats options.
   Details:
    * configuration.stats.colors should be a boolean.
    * configuration.stats.colors has an unknown property 'level'. These properties are valid:
      object { bold?, red?, green?, cyan?, magenta?, yellow? }
    * configuration.stats.colors has an unknown property 'hasBasic'. These properties are valid:
      object { bold?, red?, green?, cyan?, magenta?, yellow? }
    * configuration.stats.colors has an unknown property 'has256'. These properties are valid:
      object { bold?, red?, green?, cyan?, magenta?, yellow? }
    * configuration.stats.colors has an unknown property 'has16m'. These properties are valid:
      object { bold?, red?, green?, cyan?, magenta?, yellow? }
    * configuration.stats.colors should be one of these:
      boolean | object { bold?, red?, green?, cyan?, magenta?, yellow? }
    * configuration.stats should be a boolean.
    * configuration.stats should be one of these:
      "none" | "errors-only" | "minimal" | "normal" | "detailed" | "verbose"

The easy fix is to not mutate the stats object, so webpack isn't breaking with a fatal error. But we may also consider setting a valid colors property.

Does this PR introduce a breaking change?
No

Other information
Please see this minimal project to reproduce: https://gist.github.com/BenoitZugmeyer/29810768cc0f7a4413265d285a160027

The webpack configuration `stats` property has the same format as the
`devServer.stats` property, so the same object is frequently reused to
define the same stats output.

Since webpack v3.8.0, the `stats` property is validated more strictly by
webpack.  webpack-dev-server is mutating this `stats` object by adding a
`colors` property, which is an object and doesn't match the webpack
configuration schema, causing a fatal error.

The easy fix is to *not* mutate the `stats` object, so webpack isn't
breaking with a fatal error.  But we may also consider setting a valid
`colors` property.
@codecov
Copy link

@codecov codecov bot commented Nov 3, 2017

Codecov Report

Merging #1174 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1174   +/-   ##
=======================================
  Coverage   76.31%   76.31%           
=======================================
  Files           5        5           
  Lines         477      477           
  Branches      154      154           
=======================================
  Hits          364      364           
  Misses        113      113

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef18fc8...3b2a76a. Read the comment docs.

@@ -294,7 +294,9 @@ function processOptions(webpackOptions) {
};
}

if (typeof options.stats === 'object' && typeof options.stats.colors === 'undefined') { options.stats.colors = argv.color; }
if (typeof options.stats === 'object' && typeof options.stats.colors === 'undefined') {
options.stats = Object.assign({}, options.stats, { colors: argv.color });

This comment has been minimized.

@shellscape

shellscape Nov 3, 2017
Contributor

I may be having a brain fart here, so please correct me if I'm wrong. this doesn't look like it'll avoid mutating the stats property. there's a larger issue here (which is resolved in the v3 beta branch) of the options passed being mutated on the whole:

var a = { stats: {} };

function doit (o) {
  o.stats = Object.assign({}, a.stats, { color: '0' });
}

doit(a);
a;

// returns
// {stats: {…}}
//   stats: {color: "0"}
//   __proto__: Object

I could be missing something, but I think the solution is to prevent the top level options object passed to devServer from being mutated.

This comment has been minimized.

@BenoitZugmeyer

BenoitZugmeyer Nov 3, 2017
Author Contributor

You are right, but it's the stats property value that shouldn't be mutated.

var stats = {};
var a = { stats: stats };

function doit (o) {
  o.stats = Object.assign({}, a.stats, { color: '0' });
}

doit(a);
stats;

// {}

This comment has been minimized.

@shellscape

shellscape Nov 3, 2017
Contributor

I see, I see. Hadn't considered that case. Could you put a simple test for this in /test?

This comment has been minimized.

@BenoitZugmeyer

BenoitZugmeyer Nov 3, 2017
Author Contributor

Sadly I cannot write a good test case because when running test/cli.test.js, the package supports-color detect that it doesn't run in a tty, so it returns false (which webpack sees as valid).

shellscape added 2 commits Dec 22, 2017
@shellscape
Copy link
Contributor

@shellscape shellscape commented Dec 22, 2017

@BenoitZugmeyer thanks for your patience on this one. It's now merged.

@shellscape shellscape merged commit 32c3ceb into webpack:master Dec 22, 2017
5 checks passed
5 checks passed
Codacy/PR Quality Review Good work! A positive pull request.
Details
codecov/patch Coverage not affected when comparing ef18fc8...3b2a76a
Details
codecov/project 76.31% remains the same compared to ef18fc8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants