Improvement: add an option to watch messaging. Add .idea to .gitignore #200

EugeneHlushko · 2017-12-12T09:15:27Z

What kind of change does this PR introduce?

Adds infoVerbosity option which is set to info by default. In order for user to be able to control the amount of extra messaging output.

Provides clear indication of when compilation had started and when it had finished if such verbosity is requested

Fixes webpack/webpack#6051 and webpack/webpack#5895

Did you add tests for your changes?
Yes
Also added PR for json schema validation of the option: webpack/webpack#6332
UPD: we wont add these to webpkac/webpack's schema, instead we can create our own in webpack-cli

If relevant, did you update the documentation?
Not yet had been approved

Summary

See webpack/webpack#6051 and webpack/webpack#5895

Does this PR introduce a breaking change?

no

Other information

EugeneHlushko · 2017-12-12T09:16:16Z

Hi @sokra, here's is migrated pr to webpack-cli. Couple of tests are now failing due to message being off by default. Should i turn it on by default or should i change the tests to not expect the messages when the param is not there?

evenstensberg · 2017-12-12T09:21:15Z

@EugeneHlushko update the tests, would be convenient if you add a way to test this locally, not just via tests too / screenshot 👍

EugeneHlushko · 2017-12-12T10:42:59Z

Hi @ev1stensberg

i've updated the tests with config to expect the string and default ones to not expect it. Do you think that is enough or should i just add this option to all old tests and create a separate test case to see that string?

EugeneHlushko · 2017-12-14T08:47:01Z

Any update on this one @ev1stensberg ?

evenstensberg · 2017-12-14T11:36:14Z

Is there an issue on this? If not , you'd have to open one and @sokra must approve the feature
Edit: Could you link the absolute URL of the bugs you're fixing?

sokra · 2017-12-14T14:23:13Z

See also webpack/webpack#5895

I guess it would a a good idea to combine these features into something more general like info-verbosity...

evenstensberg

A few things here. Don't modify a binTestCase test, create a new one for your feature. For the alias, it should be more discussed per @sokra 's saying, maybe we can combine some logic

EugeneHlushko · 2017-12-14T15:03:52Z

Got your point @sokra
info-verbosity sounds more like a level setting rather than a bool. In that case i would make it: verbose and silent in this PR, and in future we could always add third option and so on. Otherwise vote for a bool^^

And by default it would be set to verbose per @ev1stensberg 's request.

If you are ok with that the plan for me would be adding that option within the scope of this PR, adding additional test case for silent mode. And the next PR would then cover more messages like Build started... and Build finished in the watch mode.

Please comment or approve the course.

P.S. Updating the description with permalinks to issues

evenstensberg · 2017-12-14T16:33:08Z

@EugeneHlushko @sokra I actually think that we should try to align this with #22 , as this is the same issue?

EugeneHlushko · 2017-12-14T18:19:48Z

Not exactly the same but similar @ev1stensberg . If you read the issues, its more about being able to not see extra text except for the compilation output, and another one wants to see clear messages on start and end of the compilation process. But we indeed can also respect this flag for hash and version etc. Can include it into the scope of current PR, what you think?

Just to be clear, i think templates of the stats output is out of scope.

evenstensberg · 2017-12-14T21:05:35Z

I think you should consult with @sokra, it would be nice if we'd combine these features into one alias

EugeneHlushko · 2017-12-15T08:55:17Z

Ok lets see what @sokra can say on this thread after what i've wrote yesterday: #200 (comment)

EugeneHlushko · 2017-12-17T10:29:45Z

Hi @sokra , @ev1stensberg
As this isnt getting additional comments i've implemented the last suggested solution and added a new test case. Please review

EugeneHlushko · 2017-12-22T09:25:18Z

Any updates on this PR? the two issues opened are not getting closed if this is freezed. @sokra @ev1stensberg

sokra · 2017-12-22T10:42:36Z

info-verbosity sounds more like a level setting rather than a bool.

yep, in my opinion it should be a level setting.

I would default it to "info".

In "verbose" you could add the messages for start, end invalid...

evenstensberg

Tobias left some comments, also the .gitignore should be removed from commit, as the idea ignore has been implemented already

EugeneHlushko · 2017-12-26T13:23:08Z

Hi @ev1stensberg and @sokra
I did update the PR accordingly

EugeneHlushko · 2018-01-03T11:39:21Z

Should i rebase or is this still under discussion @sokra ?

sokra · 2018-01-03T21:24:49Z

bin/webpack.js

@@ -378,6 +384,10 @@ yargs.parse(process.argv.slice(2), (err, argv, output) => {
 			}
 		});

+		ifArg("info-verbosity", function(value) {
+			outputOptions.infoVerbosity = value;


You don't have to put this into outputOptions. A local variable would be fine.

evenstensberg · 2018-01-04T05:26:31Z

Could you rebase? @EugeneHlushko

EugeneHlushko · 2018-01-04T11:12:56Z

Yup, on it. I will leave it in outputOptions as i thought to use it in the plugins too, but if you insist i can change to local var

EugeneHlushko · 2018-01-04T11:25:01Z

done @ev1stensberg

EugeneHlushko · 2018-01-18T12:59:22Z

But i did add that, or do you mean anything else?
Here:
https://github.com/webpack/webpack-cli/pull/200/files#diff-877a99a662829398e8136ee938cf1c93R443

…ck into two places

EugeneHlushko · 2018-01-19T08:59:08Z

Hi @ev1stensberg
Forgot to mention you in the last comment.
Verbose is already added in previous commits:
https://github.com/webpack/webpack-cli/pull/200/files#diff-877a99a662829398e8136ee938cf1c93R443

evenstensberg · 2018-01-20T16:51:14Z

Let me take another look later tonight

EugeneHlushko · 2018-01-21T16:44:43Z

Sure thing mate, looking forward to feedback if any @ev1stensberg

evenstensberg

One small tweak and we're set

evenstensberg · 2018-01-21T16:54:12Z

bin/webpack.js

@@ -399,6 +405,12 @@
 				}
 			});

+			ifArg("info-verbosity", function(value) {
+				if (typeof value !== "string" || !["none", "info", "verbose"].includes(value))
+					throw new Error("Invalid configuration object. \n configuration['info-verbosity'] should be one of these:\n \"none\" | \"info\" | \"verbose\"");


I think we've got a error helper for this

I maybe misunderstood, but couldnt find the usage in codebase of error helper, could you please point me into the right direction here? All i've found is bin/errorHelpers.js which basically works with existing error but doesnt throw one for me

Hm, okay, so if you look in the function inside the bin folder that I believe is called convert-argv, there's a processing of the options, and if it's not matching the schema, it throws. At least that is what I think it's doing under the hood. I'm not on a computer right now, so let me get back to you in a bit

yeah but seems it doesnt support enum which could substitue ![].includes call as jsonschema does?

It should only be a string

EugeneHlushko · 2018-01-22T16:29:37Z

bin/webpack.js

@@ -399,6 +405,12 @@
 				}
 			});

+			ifArg("info-verbosity", function(value) {
+				if (!["none", "info", "verbose"].includes(value))


@ev1stensberg can we keep this check until schema lands so there is no misuse?

Could you create an issue for it? Mergeable after

done #249 @ev1stensberg

evenstensberg · 2018-01-22T22:11:32Z

Do you mind adding webpack-dev-server as the same dependency as webpack? Seems to get linting errors

…arning for webpack-dev-server

EugeneHlushko · 2018-01-23T10:44:55Z

@ev1stensberg added webpack-dev-server to allowModules section

webpack-bot · 2018-01-23T10:55:18Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

evenstensberg

Superb! Thanks! LGTM

evenstensberg self-requested a review December 12, 2017 09:21

evenstensberg added Chill ❄️ CLI labels Dec 13, 2017

evenstensberg requested changes Dec 14, 2017

View reviewed changes

EugeneHlushko mentioned this pull request Dec 18, 2017

silent mode webpack/webpack#6051

Closed

evenstensberg requested changes Dec 22, 2017

View reviewed changes

EugeneHlushko force-pushed the issue-6051 branch from 4a5fda4 to 8deea67 Compare December 26, 2017 13:02

EugeneHlushko force-pushed the issue-6051 branch from 343ccbb to 4862525 Compare December 26, 2017 14:07

sokra approved these changes Jan 3, 2018

View reviewed changes

evenstensberg approved these changes Jan 4, 2018

View reviewed changes

EugeneHlushko force-pushed the issue-6051 branch from 4862525 to 135f407 Compare January 4, 2018 11:24

webpack-bot added PR: reviewed-changes-requested PR: CI-not-ok and removed PR: CI-ok labels Jan 18, 2018

Improvement: add an option for info-verbosity: dont split verbose che…

261d54f

…ck into two places

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

evenstensberg requested changes Jan 21, 2018

View reviewed changes

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

Improvement: add an option for info-verbosity: remove redundant typeof

0b05716

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

EugeneHlushko commented Jan 22, 2018

View reviewed changes

Improvement: add an option for info-verbosity: get rid of unpublish w…

2695ec1

…arning for webpack-dev-server

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jan 23, 2018

ematipico approved these changes Jan 23, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jan 23, 2018

evenstensberg approved these changes Jan 23, 2018

View reviewed changes

evenstensberg merged commit 3aba841 into webpack:master Jan 23, 2018

EugeneHlushko deleted the issue-6051 branch January 26, 2018 15:30

EugeneHlushko mentioned this pull request Feb 2, 2018

Update docs with info-verbosity option webpack/webpack.js.org#1817

Merged

Improvement: add an option to watch messaging. Add .idea to .gitignore #200

Improvement: add an option to watch messaging. Add .idea to .gitignore #200

Conversation

EugeneHlushko commented Dec 12, 2017 • edited Loading

EugeneHlushko commented Dec 12, 2017

evenstensberg commented Dec 12, 2017

EugeneHlushko commented Dec 12, 2017

EugeneHlushko commented Dec 14, 2017

evenstensberg commented Dec 14, 2017 • edited Loading

sokra commented Dec 14, 2017

evenstensberg left a comment • edited Loading

Choose a reason for hiding this comment

EugeneHlushko commented Dec 14, 2017 • edited Loading

evenstensberg commented Dec 14, 2017

EugeneHlushko commented Dec 14, 2017 • edited Loading

evenstensberg commented Dec 14, 2017

EugeneHlushko commented Dec 15, 2017

EugeneHlushko commented Dec 17, 2017

EugeneHlushko commented Dec 22, 2017

sokra commented Dec 22, 2017 • edited Loading

evenstensberg left a comment

Choose a reason for hiding this comment

EugeneHlushko commented Dec 26, 2017

EugeneHlushko commented Jan 3, 2018

Choose a reason for hiding this comment

evenstensberg commented Jan 4, 2018

EugeneHlushko commented Jan 4, 2018

EugeneHlushko commented Jan 4, 2018

EugeneHlushko commented Jan 18, 2018 • edited Loading

EugeneHlushko commented Jan 19, 2018

evenstensberg commented Jan 20, 2018

EugeneHlushko commented Jan 21, 2018

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

evenstensberg commented Jan 22, 2018

EugeneHlushko commented Jan 23, 2018

webpack-bot commented Jan 23, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

EugeneHlushko commented Dec 12, 2017 •

edited

Loading

evenstensberg commented Dec 14, 2017 •

edited

Loading

evenstensberg left a comment •

edited

Loading

EugeneHlushko commented Dec 14, 2017 •

edited

Loading

EugeneHlushko commented Dec 14, 2017 •

edited

Loading

sokra commented Dec 22, 2017 •

edited

Loading

EugeneHlushko commented Jan 18, 2018 •

edited

Loading