-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Improvement: add an option to watch messaging. Add .idea to .gitignore #200
Conversation
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? |
@EugeneHlushko update the tests, would be convenient if you add a way to test this locally, not just via tests too / screenshot 👍 |
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? |
Any update on this one @ev1stensberg ? |
Is there an issue on this? If not , you'd have to open one and @sokra must approve the feature |
See also webpack/webpack#766 I guess it should be on by default. See also webpack/webpack#5895 I guess it would a a good idea to combine these features into something more general like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Got your point @sokra And by default it would be set to 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 Please comment or approve the course. P.S. Updating the description with permalinks to issues |
@EugeneHlushko @sokra I actually think that we should try to align this with #22 , as this is the same issue? |
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 Just to be clear, i think templates of the stats output is out of scope. |
I think you should consult with @sokra, it would be nice if we'd combine these features into one alias |
Ok lets see what @sokra can say on this thread after what i've wrote yesterday: #200 (comment) |
Hi @sokra , @ev1stensberg |
Any updates on this PR? the two issues opened are not getting closed if this is freezed. @sokra @ev1stensberg |
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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tobias left some comments, also the .gitignore
should be removed from commit, as the idea ignore has been implemented already
4a5fda4
to
8deea67
Compare
Hi @ev1stensberg and @sokra |
343ccbb
to
4862525
Compare
Should i rebase or is this still under discussion @sokra ? |
bin/webpack.js
Outdated
@@ -378,6 +384,10 @@ yargs.parse(process.argv.slice(2), (err, argv, output) => { | |||
} | |||
}); | |||
|
|||
ifArg("info-verbosity", function(value) { | |||
outputOptions.infoVerbosity = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to put this into outputOptions
. A local variable would be fine.
Could you rebase? @EugeneHlushko |
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 |
4862525
to
135f407
Compare
done @ev1stensberg |
But i did add that, or do you mean anything else? |
…ck into two places
Hi @ev1stensberg |
Let me take another look later tonight |
Sure thing mate, looking forward to feedback if any @ev1stensberg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small tweak and we're set
bin/webpack.js
Outdated
@@ -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\""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've got a error helper for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but seems it doesnt support enum which could substitue ![].includes
call as jsonschema does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should only be a string
@@ -399,6 +405,12 @@ | |||
} | |||
}); | |||
|
|||
ifArg("info-verbosity", function(value) { | |||
if (!["none", "info", "verbose"].includes(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ev1stensberg can we keep this check until schema lands so there is no misuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create an issue for it? Mergeable after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done #249 @ev1stensberg
Do you mind adding |
…arning for webpack-dev-server
@ev1stensberg added |
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superb! Thanks! LGTM
What kind of change does this PR introduce?
Adds
infoVerbosity
option which is set toinfo
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