fix(Server): cascade stats options

[yoannmoinet]

• This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

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]

All committers have signed the CLA.

[alexander-akait]

Great work, can you add tests?

[alexander-akait]

Also please accept CLA

[yoannmoinet]

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

[codecov]

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]

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

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]

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

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

[yoannmoinet]

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?

[alexander-akait]

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

[yoannmoinet]

this is where I'm using it

[alexander-akait]

Loooks good, need add tests for warningsFilter

[hiroppy]

The snapshot test is currently failing.

[yoannmoinet]

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]

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

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

[yoannmoinet]

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

[yoannmoinet]

I'm not sure I understand Travis' errors.

[yoannmoinet]

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

[alexander-akait]

@yoannmoinet try to do rebase (master)

[yoannmoinet]

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]

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

[yoannmoinet]

Oh, good point, will update.

[yoannmoinet]

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

[yoannmoinet]

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 ?

[alexander-akait]

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

[alexander-akait]

/cc @hiroppy

[hiroppy]

thanks!