fix(Server): cascade stats options #1665

yoannmoinet · 2019-02-14T13:49:34Z

For Bugs and Features; did you add new tests?

Will work on it if really needed.

Motivation / Use-Case

We get flooded by warnings in the console.

Additional Info

Fixes #1557

jsf-clabot · 2019-02-14T13:49:36Z

All committers have signed the CLA.

alexander-akait

Great work, can you add tests?

alexander-akait · 2019-02-14T13:50:43Z

Also please accept CLA

yoannmoinet · 2019-02-14T14:09:22Z

Actually this won't work if we use a string for options.stats like errors-only.
Will update and add tests.

codecov · 2019-02-14T14:47:10Z

Codecov Report

Merging #1665 into master will increase coverage by 0.05%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1665      +/-   ##
==========================================
+ Coverage   86.08%   86.13%   +0.05%     
==========================================
  Files           8        8              
  Lines         539      541       +2     
  Branches      162      162              
==========================================
+ Hits          464      466       +2     
  Misses         62       62              
  Partials       13       13

Impacted Files	Coverage Δ
lib/utils/createConfig.js	`95.5% <100%> (ø)`	⬆️
lib/Server.js	`82.29% <80%> (+0.09%)`	⬆️

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 7ea9ab9...049ca87. Read the comment docs.

yoannmoinet · 2019-02-14T15:07:29Z

@evilebottnawi I've refactored the code to also accepts strings.
I tried to write tests, but I'm not sure where to begin.

I tried creating a new test/Server.test.js but I can't make any test pass, I think I'm going in the wrong direction.
Would you help please?

yoannmoinet · 2019-02-26T09:05:01Z

Can someone help with the tests (cc @evilebottnawi) ?

Here's what I tried with Server.test.js but I've been unsuccessful.

'use strict';

const webpack = require('webpack');
const Server = require('../lib/Server');
const config = require('./fixtures/simple-config/webpack.config');

const allStats = [
  {},
  // eslint-disable-next-line
  undefined,
  'errors-only',
  {
    assetsSort: "size"
  }
];

describe('Server', () => {
  allStats.forEach((s) => {
    it('should cascade stats options', (done) => {
      const compiler = webpack(Object.assign({}, config, s && { stats: s }));
      const server = new Server(compiler, {});
      const next = () => {
        const localStats = server.getStats(server._stats);

        if(!s || !Object.keys(s).length) {
          expect(localStats).toBe(server.DEFAULT_STATS);
        } else {
          expect(localStats).not.toBe(server.DEFAULT_STATS);
        }

        server.close(() => {
          done();
        });
      };
      server.listen(8080, 'localhost', (err) => {
        if (err) return next(err);
        next();
      });
    });
  });
});

alexander-akait

Invalid place for solution, see https://github.com/webpack/webpack-dev-server/blob/master/lib/utils/createConfig.js#L107 and https://github.com/webpack/webpack-dev-server/blob/master/lib/utils/createConfig.js#L118, we should get firstWpOpt.stats (if define)

yoannmoinet · 2019-02-28T14:06:01Z

@evilebottnawi from what I understand, what you pointed to is if we want to use webpack's stats config if none is given to devServer's config, as a fallback.
I've added a commit that does just that, but on its own, it won't fix the bug, we still need the change in Server.js for it to cascade into the browser's console.

alexander-akait · 2019-03-04T09:10:17Z

lib/Server.js

+      warnings: true,
+      errors: true,
+      errorDetails: false,
+    };


Let's avoid this.DEFAULT_STATS (because it should be as static private property)

I needed it for the tests I'm trying to write for Server.js.
Should I add this as a public static of the Server class?

Can you provide link where you need it for test? You already have getStats method

this is where I'm using it

alexander-akait

Loooks good, need add tests for warningsFilter

hiroppy · 2019-03-05T21:24:22Z

The snapshot test is currently failing.

yoannmoinet · 2019-03-06T09:41:56Z

Loooks good, need add tests for warningsFilter

@evilebottnawi I'm not sure what you're asking for.

The snapshot test is currently failing.

@hiroppy indeed, I cannot reproduce locally though, so I don't know what to look for.

alexander-akait · 2019-03-06T09:49:42Z

@yoannmoinet we need add tests for stats https://webpack.js.org/configuration/stats/ (warningsFilter option) to ensure we fix problem (what is written in issue)

yoannmoinet · 2019-03-06T09:53:06Z

@evilebottnawi oh yes, I'm working on this right now 😉
I wasn't sure we were talking about the same thing 👍

yoannmoinet · 2019-03-07T14:15:42Z

@evilebottnawi here you go, I've added test for the Server.

yoannmoinet · 2019-03-08T10:40:56Z

I'm not sure I understand Travis' errors.

yoannmoinet · 2019-03-12T17:10:12Z

can you help with the failing task in Travis @evilebottnawi please?

alexander-akait · 2019-03-12T17:11:17Z

@yoannmoinet try to do rebase (master)

yoannmoinet · 2019-03-18T16:25:21Z

I'm not sure what kind of test I'm supposed to add following the codecov/patch task failing.
I felt like I added all the tests needed.
cc @evilebottnawi

alexander-akait

Good job, one note

alexander-akait · 2019-03-18T16:30:26Z

lib/Server.js

@@ -668,6 +674,10 @@ class Server {
    }, this);
  }

+  getStats(statsObj) {
+    return statsObj.toJson(this.stats);
+  }


Let's move

this.stats = options.stats && Object.keys(options.stats).length ? options.stats : Server.DEFAULT_STATS;

logic here, we don't need new property like stats

Oh, good point, will update.

I still need to keep the property this.stats to have access to options.stats.

Not sure it would be useful to move the logic since I still need to keep that this.stats so I can use options.stats in getStats().
What do you think @evilebottnawi ?

Yep, you are right, we should rewrite this in next major release

alexander-akait · 2019-03-18T17:09:12Z

/cc @hiroppy

hiroppy

thanks!

yoannmoinet requested review from alexander-akait and michael-ciniawsky as code owners February 14, 2019 13:49

alexander-akait reviewed Feb 14, 2019

View reviewed changes

alexander-akait closed this Feb 14, 2019

alexander-akait reopened this Feb 14, 2019

yoannmoinet force-pushed the master branch from cb53ba7 to dfd4826 Compare February 14, 2019 15:06

alexander-akait requested changes Feb 26, 2019

View reviewed changes

yoannmoinet requested a review from hiroppy as a code owner February 28, 2019 14:06

alexander-akait reviewed Mar 4, 2019

View reviewed changes

alexander-akait requested changes Mar 4, 2019

View reviewed changes

yoannmoinet force-pushed the master branch from 6929f45 to ff310d8 Compare March 7, 2019 16:36

yoannmoinet added 4 commits March 12, 2019 18:14

fix(Server): cascade stats options

05ca937

fix(config): use webpack's stats if none is submitted via config

cea1eb8

fix(Server): use DEFAULT_STATS as static

18231ba

test(Server): add tests for the stats cascading

f3f590a

test(config): update snapshots

01ba5f4

yoannmoinet force-pushed the master branch from ff310d8 to 01ba5f4 Compare March 12, 2019 17:14

alexander-akait requested changes Mar 18, 2019

View reviewed changes

alexander-akait approved these changes Mar 18, 2019

View reviewed changes

hiroppy approved these changes Mar 20, 2019

View reviewed changes

evilebottnawi added 2 commits March 21, 2019 13:09

Merge branch 'master' into master

819ed21

Merge branch 'master' into master

049ca87

alexander-akait merged commit efaa740 into webpack:master Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Server): cascade stats options #1665

fix(Server): cascade stats options #1665

yoannmoinet commented Feb 14, 2019

jsf-clabot commented Feb 14, 2019 •

edited

Loading

alexander-akait left a comment

alexander-akait commented Feb 14, 2019

yoannmoinet commented Feb 14, 2019

codecov bot commented Feb 14, 2019 •

edited

Loading

yoannmoinet commented Feb 14, 2019

yoannmoinet commented Feb 26, 2019

alexander-akait left a comment

yoannmoinet commented Feb 28, 2019

alexander-akait Mar 4, 2019 •

edited

Loading

yoannmoinet Mar 6, 2019 •

edited

Loading

alexander-akait Mar 6, 2019

yoannmoinet Mar 7, 2019

alexander-akait left a comment

hiroppy commented Mar 5, 2019

yoannmoinet commented Mar 6, 2019

alexander-akait commented Mar 6, 2019

yoannmoinet commented Mar 6, 2019

yoannmoinet commented Mar 7, 2019

yoannmoinet commented Mar 8, 2019

yoannmoinet commented Mar 12, 2019

alexander-akait commented Mar 12, 2019 •

edited

Loading

yoannmoinet commented Mar 18, 2019

alexander-akait left a comment

alexander-akait Mar 18, 2019

yoannmoinet Mar 18, 2019

yoannmoinet Mar 18, 2019 •

edited

Loading

yoannmoinet Mar 18, 2019

alexander-akait Mar 18, 2019

alexander-akait commented Mar 18, 2019

hiroppy left a comment

fix(Server): cascade stats options #1665

fix(Server): cascade stats options #1665

Conversation

yoannmoinet commented Feb 14, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Additional Info

jsf-clabot commented Feb 14, 2019 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Feb 14, 2019

yoannmoinet commented Feb 14, 2019

codecov bot commented Feb 14, 2019 • edited Loading

Codecov Report

yoannmoinet commented Feb 14, 2019

yoannmoinet commented Feb 26, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

yoannmoinet commented Feb 28, 2019

alexander-akait Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

yoannmoinet Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

alexander-akait Mar 6, 2019

Choose a reason for hiding this comment

yoannmoinet Mar 7, 2019

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

hiroppy commented Mar 5, 2019

yoannmoinet commented Mar 6, 2019

alexander-akait commented Mar 6, 2019

yoannmoinet commented Mar 6, 2019

yoannmoinet commented Mar 7, 2019

yoannmoinet commented Mar 8, 2019

yoannmoinet commented Mar 12, 2019

alexander-akait commented Mar 12, 2019 • edited Loading

yoannmoinet commented Mar 18, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait Mar 18, 2019

Choose a reason for hiding this comment

yoannmoinet Mar 18, 2019

Choose a reason for hiding this comment

yoannmoinet Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

yoannmoinet Mar 18, 2019

Choose a reason for hiding this comment

alexander-akait Mar 18, 2019

Choose a reason for hiding this comment

alexander-akait commented Mar 18, 2019

hiroppy left a comment

Choose a reason for hiding this comment

jsf-clabot commented Feb 14, 2019 •

edited

Loading

codecov bot commented Feb 14, 2019 •

edited

Loading

alexander-akait Mar 4, 2019 •

edited

Loading

yoannmoinet Mar 6, 2019 •

edited

Loading

alexander-akait commented Mar 12, 2019 •

edited

Loading

yoannmoinet Mar 18, 2019 •

edited

Loading