Skip to content
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

Merged
merged 8 commits into from
Jan 23, 2018
24 changes: 23 additions & 1 deletion bin/webpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,12 @@
type: "boolean",
group: DISPLAY_GROUP,
describe: "Show more details"
},
"info-verbosity": {
type: "string",
default: "info",
group: DISPLAY_GROUP,
describe: "Controls the output of lifecycle messaging e.g. Started watching files... (verbose, info, none)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between info and verbose ? Seems to do the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well within this PR yes, but the next one should add two more messages they will be verbose only (see linked issues)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make it in this PR as well? Ask any of the maintainers if you got any questions!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it will stretch out for another month in that case, any good reason for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving code unfinished is a bad habit 💃

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if you promise to respond quickly im on it! :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinky Promise 😆

}
});

Expand Down Expand Up @@ -399,6 +405,12 @@
}
});

ifArg("info-verbosity", function(value) {
if (!["none", "info", "verbose"].includes(value))
Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new Error("Invalid configuration object. \n configuration['info-verbosity'] should be one of these:\n \"none\" | \"info\" | \"verbose\"");
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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

outputOptions.infoVerbosity = value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If info-verbosity is not a string, i.e boolean, it will still output. Would be nice with an error that will match against webpack-schema. https://github.com/webpack/webpack/blob/master/schemas/webpackOptionsSchema.json

You should send a PR to add info-verbosity and link it to this.

Copy link
Member Author

@EugeneHlushko EugeneHlushko Jan 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will modify within this scope (the schema), completely forgot about it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schema added and pr's linked in description section

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool beans 🐻

});

var webpack = require("webpack/lib/webpack.js");

var lastHash = null;
Expand Down Expand Up @@ -428,6 +440,15 @@
);
}

if (outputOptions.infoVerbosity === "verbose") {
compiler.hooks.beforeCompile.tap("WebpackInfo", (compilation) => {
console.log("\nCompilation starting…\n");
});
compiler.hooks.afterCompile.tap("WebpackInfo", (compilation) => {
console.log("\nCompilation finished\n");
});
}

function compilerCallback(err, stats) {
if (!options.watch || err) {
// Do not keep cache anymore
Expand Down Expand Up @@ -465,7 +486,8 @@
process.stdin.resume();
}
compiler.watch(watchOptions, compilerCallback);
console.log("\nWebpack is watching the files…\n");
if (outputOptions.infoVerbosity !== "none")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

console.log("\nWebpack is watching the files…\n");
} else compiler.run(compilerCallback);
}

Expand Down
Empty file.
7 changes: 7 additions & 0 deletions test/binCases/errors/info-verbosity/stdin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
"use strict";

module.exports = function testAssertions(code, stdout, stderr) {
expect(stderr[3]).toContain("Error: Invalid configuration object.");
expect(stderr[4]).toContain("configuration['info-verbosity'] should be one of these:");
expect(stderr[5]).toContain("\"none\" | \"info\" | \"verbose\"");
};
5 changes: 5 additions & 0 deletions test/binCases/errors/info-verbosity/test.opts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
--entry ./index.js
--config ./webpack.config.js
--output-filename [name].js
--output-chunk-filename [id].chunk.js
--info-verbosity false
5 changes: 5 additions & 0 deletions test/binCases/errors/info-verbosity/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
var path = require("path");

module.exports = {
entry: path.resolve(__dirname, "./index")
};
Empty file.
Empty file.
11 changes: 11 additions & 0 deletions test/binCases/watch/info-verbosity-off/stdin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
"use strict";

module.exports = function testAssertions(stdout, stderr, done) {
expect(stdout).toEqual(expect.anything());
expect(stdout[0]).toContain("");
expect(stdout[1]).not.toContain("Webpack is watching the files…");
expect(stdout[1]).toContain("Version: webpack");

expect(stderr).toHaveLength(0);
done();
};
7 changes: 7 additions & 0 deletions test/binCases/watch/info-verbosity-off/test.opts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
--entry ./index.js
--config ./webpack.config.js
--output-filename [name].js
--output-chunk-filename [id].chunk.js
--target async-node
--watch
--info-verbosity none
5 changes: 5 additions & 0 deletions test/binCases/watch/info-verbosity-off/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
var path = require("path");

module.exports = {
entry: path.resolve(__dirname, "./index")
};
Empty file.
Empty file.
17 changes: 17 additions & 0 deletions test/binCases/watch/info-verbosity-verbose/stdin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
"use strict";

module.exports = function testAssertions(stdout, stderr, done) {
expect(stdout).toEqual(expect.anything());
expect(stdout[0]).toContain("");
expect(stdout[1]).toContain("Compilation starting…");
expect(stdout[2]).toContain("");
expect(stdout[3]).toContain("");
expect(stdout[4]).toContain("Webpack is watching the files…");
expect(stdout[5]).toContain("");
expect(stdout[6]).toContain("");
expect(stdout[7]).toContain("Compilation finished");
expect(stdout[8]).toContain("");

expect(stderr).toHaveLength(0);
done();
};
7 changes: 7 additions & 0 deletions test/binCases/watch/info-verbosity-verbose/test.opts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
--entry ./index.js
--config ./webpack.config.js
--output-filename [name].js
--output-chunk-filename [id].chunk.js
--target async-node
--watch
--info-verbosity verbose
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
var path = require("path");

module.exports = {
entry: path.resolve(__dirname, "./index")
};