Convert Stats into Pluggable API #6702

nveenjain · 2018-03-07T19:27:47Z

What kind of change does this PR introduce?

It is a new feature.

User will now be able to specify something like this and it will totally be accepted:-

options = {
  stats:{
    preset:"none",
    timings:true
    }
}

The above config will only show timings at the time of output, similarly other presets can be modified too.

Moreover, We can add hooks into stats, which will allow plugin authors to set default for stats and they don't need to rely upon webpack's default stats options , they themselves can specify which stats defaults they want and can use those stats. They can also modify the printing of stats(choose which stats options to output). Along with this, we ourselves can let defaults be defined using some presets/plugins instead of setting them from Stats.js file.

Note that plugins only set the defaults, meaning they are to be treated as fallback values, if in case user has supplied the stats options (like in the code example above), the stats which will be shown to user will contain timing regardless of any plugin. The priority(for emitting stats) is:-

User supplied stats option > User supplied Plugins > Presets/defaults

Did you add tests for your changes?
YES

If relevant, link to documentation update:
Will update docs, once this feature is good to go.

Summary
Added preset option support to stats in options object. Now preset can also be passed along with some other options, for user convinience.

Added hooks into stats, and turned stats into pluggable API, Now the defaults are loaded by using plugins in stats, These defaults can also be provided by external plugin authors. Note that this will change defaults only, if user has supplied options himself, then it won't change anything. Also added type checking to options supplied, if the options supplied are not of supported types, then we will fallback to default values. If the default value ( by external plugin) is not supported, then an error will be thrown. This will make stats less prone to unwanted errors. Please not that by default we supply our internal plugin and use it for stats and hence default will not throw errors.

Does this PR introduce a breaking change?
No.

Other information

webpack-bot · 2018-03-09T18:15:24Z

The minimum test ratio has been reached. Thanks!

nveenjain · 2018-03-10T16:04:09Z

Also, plugin authors can hook into done of compiler to manipulate the stats which are to be printed. If this PR is merged, then i would love to contribute to docs too for this feature.

sokra

That's a good start, but currently the hooks are not very useful.

Try to create a plugin lib/stats/AssetsStatsPlugin.js and move everything related to assets from lib/Stats.js into this plugin. Apply this plugin by default in lib/WebpackOptionsApply.js. The Stats should no longer know about assets.

In the first step you can do this only for toJson and keep toString/jsonToString.

Which hooks do you need? Which problems could occur when moving all other elements too. Is it possible for a user to write custom plugins too?

Can the interpretation of the options be moved to WebpackOptionsApply.js? (Other "options" are interpreted here and plugins are applied conditionally. Can we stick the the same schema for stats options too?)

sokra

Maybe calling

compilation.hooks.stats.call(this);

in the Stats construction would allow hooking into the stats, without a breaking change.

This would allow any plugin (added to plugins in configuration`) to hook into the Stats by using:

compiler.hooks.compilation.tap("...", (compilation) => {
  compilation.hooks.stats.tap("...", (stats) => {
    stats...
  });
});

sokra · 2018-03-13T08:09:25Z

lib/Compiler.js

@@ -144,7 +143,7 @@ class Compiler extends Tapable {
 			if (err) return callback(err);

 			if (this.hooks.shouldEmit.call(compilation) === false) {
-				const stats = new Stats(compilation);
+				const stats = compilation.getStats();


If new Stats can no longer be used, it would be a breaking change.

Should we go this way. (Would mean waiting until webpack 5)

Or can be solve this problem in a non-breaking way.

new Stats can be used, but i thought it would be better if we just called compilation.getStats() and after this call, we can refer current stats through compilation.stats and modify it accordingly, but we can surely go through non-breaking way if you suggest, Will update it.

sokra · 2018-03-13T08:14:10Z

lib/MultiStats.js

@@ -85,7 +85,7 @@ class MultiStats {

 		const obj = this.toJson(options, true);

-		return Stats.jsonToString(obj, useColors);
+		return Stats.jsonToString(obj, useColors); //TODO: convert this to non-static method


Let's ignore this for now and take care of this when finished the other problems.

nveenjain · 2018-03-13T09:22:20Z

I'll move assets to lib/Stats/AssetsStatsPlugin.js, Only hooks related to showing assets, showing cached Assets, excluding Assets and sorting Assets might be required. Then all the stuff related to assets can be moved to lib/stats/AssetsStatsPlugin.js, i think we can implement the same for modules and chunks too. Regarding interpretation of the options, I am sorry, but i think i might have misunderstood, but if we move interpretation of options to webpackOptionsApply.js, then it means that we rely on options sent by webpack.config.js everytime, however, can't we as developer, ourselves supply options to stats (while using Stats API)? in that case i don't think we'll be able to tap into the options which were not provided in webpack.config.js. I think we need to apply all the stats plugin initially in webpackOptionsApply.js. Sorry, if i misunderstood.

nveenjain · 2018-03-13T13:54:21Z

For e.g. If it is specified in config that assets should not be shown, then i may not register asset plugin( in webpackOptionsApply), but when another plugin author uses stats API, he may want default values of assets to be used, however in that case, due to asset plugin not being registered, expected things will not happen. We can keep current schema of stats options, as the plan is to do this in backwards compatible way. BTW, by moving all things related to assets, do you mean the default options of assets or anything else too?

EDIT:- Got it, will update the PR.

nveenjain · 2018-03-15T13:57:10Z

No idea on how to increase coverage further, sugesstions required 😢, unit test, but how? 😞

evenstensberg

Left some comments, mostly not related to tapable. Let's keep the mocking. Unit test are running correctly, as you've changed them. If implementation is correct, the tests should be running fine, don't you agree? For the all arrays, use optionsForPlugin or something more intuitive and extract each of the .forEach-es into general functions

evenstensberg · 2018-03-20T01:47:39Z

lib/Stats.js

+						"boolean"
+					);
+
+		const showAssets = isTypeOf(options.assets, "boolean")


From diff, looks like isTypeOf could be similar to old impl?

With this implementation, I've added type checking too, we can't allow user to pass in any value of any type, if the value supplied by user if not of the type that we require, then we fallback to options.all ( for all boolean values, except for !toString values, will come to that later), if options.all is not set, then we take into account the default values ( which user has supplied either by their custom plugins or we've supplied in webpackOptionsApply), and type check them too, if they are also not of expected type, then we throw an error. However if expected type is not boolean, then we don't fallback to options.all, we directly assert value supplied by plugins.
For the values which are dependent on toString, we first check for current value, whether current value supplied by user is of supported type or not, if not, then we fallback to plugins, if no plugins are setting any value, then we can fallback to toString.

I think type checking the options value supplied should be done. However, if you insist, we can fallback to previous implementation too.

evenstensberg · 2018-03-20T01:48:37Z

lib/Stats.js

@@ -1152,6 +1362,7 @@ class Stats {
 	}

 	static presetToOptions(name) {
+		// TODO :- DELETE THIS IN UPCOMING VERSION, WE've GOT PLUGINS :)


Bad rebase?

😄 No, actulally, it's a static method, and it's used in some test cases individually, https://github.com/webpack/webpack/blob/master/test/Stats.unittest.js#L162 . Couldn't remove it without breaking change. However we no longer need this except if some user specifically want to convert presetToOptions ( which i don't think should happen), so added to TODO to remove it in future version.

Now this can be deleted, if suggested.. No function is using it internally, only test-cases are using. Suggestions? @sokra ??

evenstensberg · 2018-03-20T01:49:06Z

lib/WebpackOptionsApply.js

@@ -59,6 +59,8 @@ const NamedChunksPlugin = require("./NamedChunksPlugin");
 const DefinePlugin = require("./DefinePlugin");
 const SizeLimitsPlugin = require("./performance/SizeLimitsPlugin");

+const StatsPlugin = require("./stats/index");


Dunno if you need to include index here, require will pick it up. At least does for us at webpack-cli.

Yups, will change it. 😄

Using multiple require would be easier to maintain. Moreover, StatsPlugin suggests that it returns a plugin, which is not the case.

evenstensberg · 2018-03-20T01:49:14Z

lib/WebpackOptionsApply.js

@@ -294,6 +296,31 @@ class WebpackOptionsApply extends OptionsApply {
 		new ImportPlugin(options.module).apply(compiler);
 		new SystemPlugin(options.module).apply(compiler);

+		switch (options.stats) {


evenstensberg · 2018-03-20T01:50:53Z

lib/stats/index.js

@@ -0,0 +1,10 @@
+exports = module.exports;


Could be exported like a regular module?

Yeah, totally. Will change it.

evenstensberg · 2018-03-20T01:53:21Z

test/Stats.unittest.js

 					}
 				});
 				const obj = mockStats.toJson();
 				obj.errors[0].should.be.equal("firstError");
 			});
 		});
+		// ?????????????????????????????????????????????????????????????????????????????????? //


Bad rebase?

Yeah, will edit that, sorry. 😃, wanted to ask question related to mocking.

evenstensberg · 2018-03-20T01:53:50Z

test/Stats.unittest.js

@@ -135,24 +167,18 @@ describe(
 					},
 					compiler: {
 						context: ""
+					},
+					hooks: {
+						stats: new SyncHook(["stats"])


Let's keep mocking it

We need hooks, as compilation is required to have hooks property now, as stats constructor will call stats hook on compilation, should, I implement a mock hook call?

…riment/stats

nveenjain · 2018-04-02T17:09:15Z

@sokra , is there any other improvements for this PR you would like to suggest? or is this PR dismissed?

alexander-akait · 2018-04-02T17:30:46Z

@nveenjain what about if i want to get only warnings and errors? What preset i should use?

alexander-akait · 2018-04-02T17:34:22Z

Also i think by default should used warnings and errors preset. A lot of problems when people don't see warnings or errors. Example uglify show warnings on mangling and if people don't see this warnings they can create issue and it is not good.

nveenjain · 2018-04-02T18:19:39Z

@evilebottnawi, i've implemented presets which were supported earlier, no new preset is introduced. For errors however, we used to use error-only preset, which is still supported. By default (when no preset is supplied) we use normal preset, which shows both warnings and errors and the default build info.

However if someone wants only error and warning, then they can pass the following options through config file:-

options.stats = {
  preset:"errrors-only",
  warnings:true
}

This will show only error and warning and nothing else. I'll document it in docs once PR is approved!

alexander-akait · 2018-04-02T18:30:50Z

@nveenjain a lot of people just use error-only and they create a lot of issue when don't see warnings.

nveenjain · 2018-04-03T06:42:34Z

@evilebottnawi , gotcha, but wouldn't preset like debug-report or bug-report be more intuitive for this purpose, I'm ready to introduce another preset fot this purpose in this PR? Or would you like to suggest some other preset. Or should I modify error-only?

alexander-akait · 2018-04-03T09:40:22Z

@nveenjain I think better to add error-only, because each warning it is can be potential error. Also it is don't break build other people after update.

nveenjain · 2018-04-03T09:55:51Z

@evilebottnawi, did it, PTAL 😃 and review please.

alexander-akait

Great job! Thanks for feedback 👍

nveenjain · 2018-04-25T13:17:44Z

@sokra @TheLarkInn can we merge this now, or is there anything missing?

TheLarkInn · 2018-05-08T04:38:55Z

@nveenjain Could you resolve conflicts and lets run CI again. This is really great work!

@sokra this looks great we should get this in. Is this a SemVer Minor change?

webpack-bot · 2018-05-08T17:15:33Z

@nveenjain Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evilebottnawi Please review the new changes.

nveenjain · 2018-05-08T18:59:17Z

@TheLarkInn resolved the conflicts, Please review.. Also, I will update the docs as soon as this PR is merged!!

Out of d8

webpack-bot · 2018-05-16T06:51:56Z

For maintainers only:

This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

sokra · 2019-07-10T12:53:53Z

done in the next branch

webpack-bot added the PR: unreviewed label Mar 7, 2018

Initial Commit for adding hooks in stats

b1b37d2

nveenjain force-pushed the experiment/stats branch from 9fe83d3 to b1b37d2 Compare March 7, 2018 19:30

webpack-bot added PR: CI-ok PR: tests-needed labels Mar 7, 2018

nveenjain changed the title ~~[WIP] Initial Commit for adding hooks in stats~~ [WIP- Suggestions Required] Initial Commit for adding hooks in stats Mar 8, 2018

Added few test cases for StatsHooks API

a2727d2

nveenjain force-pushed the experiment/stats branch from 584c018 to a2727d2 Compare March 9, 2018 17:49

webpack-bot removed the PR: tests-needed label Mar 9, 2018

nveenjain changed the title ~~[WIP- Suggestions Required] Initial Commit for adding hooks in stats~~ [Suggestions Required] Initial Commit for adding hooks in stats Mar 10, 2018

sokra requested changes Mar 13, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Mar 13, 2018

sokra requested changes Mar 13, 2018

View reviewed changes

sokra reviewed Mar 13, 2018

View reviewed changes

Convert stats into pluggable API

b1ee465

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Mar 15, 2018

nveenjain added 2 commits March 15, 2018 18:01

Merge branch 'master' into experiment/stats

511845d

Prettify Stats.js

b3fe808

nveenjain changed the title ~~[Suggestions Required] Initial Commit for adding hooks in stats~~ Commit for adding hooks in stats Mar 17, 2018

Fixed Linting

a8b4964

nveenjain force-pushed the experiment/stats branch from 51f70ec to a8b4964 Compare March 18, 2018 07:02

evenstensberg reviewed Mar 20, 2018

View reviewed changes

evenstensberg changed the title ~~Commit for adding hooks in stats~~ [WIP] Commit for adding hooks in stats Mar 20, 2018

nveenjain added 3 commits March 30, 2018 14:33

Added support for preset options in stats config

c0b0fdd

Merge branch 'master' of https://github.com/webpack/webpack into expe…

10a2480

…riment/stats

Lint file

eca15fa

nveenjain force-pushed the experiment/stats branch from 9961d2d to eca15fa Compare March 30, 2018 09:28

Added warnings to errors-only Preset

c9521ed

alexander-akait approved these changes Apr 3, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Apr 3, 2018

This was referenced Apr 3, 2018

Make it easier to render script tags with SRI data waysact/webpack-subresource-integrity#45

Closed

Add JSON mapping file waysact/webpack-subresource-integrity#74

Closed

Resolve merge conflicts

ea03342

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels May 8, 2018

sokra closed this Jul 10, 2019

nveenjain deleted the experiment/stats branch July 30, 2019 12:26

Convert Stats into Pluggable API #6702

Convert Stats into Pluggable API #6702

Conversation

nveenjain commented Mar 7, 2018 • edited

webpack-bot commented Mar 9, 2018

nveenjain commented Mar 10, 2018

sokra left a comment

Choose a reason for hiding this comment

sokra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nveenjain Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nveenjain commented Mar 13, 2018

nveenjain commented Mar 13, 2018 • edited

nveenjain commented Mar 15, 2018 • edited

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nveenjain commented Apr 2, 2018 • edited

alexander-akait commented Apr 2, 2018

alexander-akait commented Apr 2, 2018 • edited

nveenjain commented Apr 2, 2018

alexander-akait commented Apr 2, 2018

nveenjain commented Apr 3, 2018

alexander-akait commented Apr 3, 2018 • edited

nveenjain commented Apr 3, 2018 • edited

alexander-akait left a comment

Choose a reason for hiding this comment

nveenjain commented Apr 25, 2018

TheLarkInn commented May 8, 2018

webpack-bot commented May 8, 2018

nveenjain commented May 8, 2018 • edited

webpack-bot commented May 16, 2018

sokra commented Jul 10, 2019

nveenjain commented Mar 7, 2018 •

edited

nveenjain Mar 13, 2018 •

edited

nveenjain commented Mar 13, 2018 •

edited

nveenjain commented Mar 15, 2018 •

edited

nveenjain commented Apr 2, 2018 •

edited

alexander-akait commented Apr 2, 2018 •

edited

alexander-akait commented Apr 3, 2018 •

edited

nveenjain commented Apr 3, 2018 •

edited

nveenjain commented May 8, 2018 •

edited