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

cli(refactor): improve folder structure #371

Merged
merged 9 commits into from
Apr 2, 2018
4 changes: 3 additions & 1 deletion bin/process-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,9 @@ module.exports = function processOptions(yargs, argv) {
);
} else if (stats.hash !== lastHash) {
lastHash = stats.hash;
const delimiter = outputOptions.buildDelimiter ? `${outputOptions.buildDelimiter}\n` : "";
const delimiter = outputOptions.buildDelimiter
? `${outputOptions.buildDelimiter}\n`
: "";
process.stdout.write("\n" + new Date() + "\n" + "\n");
process.stdout.write(`${stats.toString(outputOptions)}\n${delimiter}`);
if (argv.s) lastHash = null;
Expand Down
18 changes: 10 additions & 8 deletions bin/webpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@
(function() {
// wrap in IIFE to be able to use return

const resolveCwd = require("resolve-cwd");
// Local version replace global one
const localCLI = resolveCwd.silent("webpack-cli/bin/webpack");
if (localCLI && localCLI !== __filename) {
require(localCLI);
const importLocal = require('import-local');
// Prefer the local installation of webpack-cli
if (importLocal(__filename)) {
return;
}
}

require("v8-compile-cache");
const ErrorHelpers = require("./errorHelpers");
Expand Down Expand Up @@ -48,7 +46,9 @@
return;
}

const yargs = require("yargs").usage(`webpack-cli ${require("../package.json").version}
const yargs = require("yargs").usage(`webpack-cli ${
require("../package.json").version
}

Usage: webpack-cli [options]
webpack-cli [options] --entry <entry> --output <output>
Expand Down Expand Up @@ -482,7 +482,9 @@ For more information, see https://webpack.js.org/api/cli/.`);
} else if (stats.hash !== lastHash) {
lastHash = stats.hash;
const statsString = stats.toString(outputOptions);
const delimiter = outputOptions.buildDelimiter ? `${outputOptions.buildDelimiter}\n` : "";
const delimiter = outputOptions.buildDelimiter
? `${outputOptions.buildDelimiter}\n`
: "";
if (statsString) stdout.write(`${statsString}\n${delimiter}`);
}
if (!options.watch && stats.hasErrors()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const { LoaderGenerator } = require("../generators/loader-generator");
* Runs a yeoman generator to create a new webpack loader project
* @returns {void}
*/

function loaderCreator() {
const env = yeoman.createEnv();
const generatorName = "webpack-loader-generator";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
const yeoman = require("yeoman-environment");
const PluginGenerator = require("../generators/plugin-generator").PluginGenerator;
const { PluginGenerator } = require("../generators/plugin-generator");

/**
* Runs a yeoman generator to create a new webpack plugin project
* @returns {void}
*/

function pluginCreator() {
const env = yeoman.createEnv();
const generatorName = "webpack-plugin-generator";
Expand Down
2 changes: 1 addition & 1 deletion lib/commands/info.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = function info() {
Binaries: ["Node", "Yarn", "npm"],
Browsers: ["Chrome", "Firefox", "Safari"],
npmPackages: "*webpack*",
npmGlobalPackages: ["webpack", "webpack-cli"],
npmGlobalPackages: ["webpack", "webpack-cli"]
})
);
};
8 changes: 6 additions & 2 deletions lib/commands/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,16 @@ const modifyHelper = require("../utils/modify-config-helper");
* First function to be called after running the init flag. This is a check,
* if we are running the init command with no arguments or if we got dependencies
*
* @param {Object} pkg - packages included when running the init command
* @param {Array} args - array of arguments such as
* packages included when running the init command
* @returns {Function} creator/npmPackagesExists - returns an installation of the package,
* followed up with a yeoman instance of that if there's packages. If not, it creates a defaultGenerator
*/

module.exports = function initializeInquirer(pkg) {
module.exports = function initializeInquirer(...args) {
const popArgs = args ? args.slice(2).pop() : null;
Copy link
Member

Choose a reason for hiding this comment

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

this means, popArgs -> Do you mean you want to pop the arguments right. ( more than one)
The 3rd argument will always have all the packages that you want to check or the packages are in argument 3, 4, 5?

const pkg = args.slice(2).length === 1 ? [] : [popArgs];

if (pkg.length === 0) {
return modifyHelper("init", defaultGenerator);
}
Expand Down
39 changes: 20 additions & 19 deletions lib/commands/migrate.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,37 @@
"use strict";

const fs = require("fs");
const path = require("path");
const chalk = require("chalk");
const diff = require("diff");
const inquirer = require("inquirer");
const PLazy = require("p-lazy");
const Listr = require("listr");

const validate = require("webpack").validate;
const WebpackOptionsValidationError = require("webpack")
.WebpackOptionsValidationError;
const { validate } = require("webpack");
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we merge this together?

const { WebpackOptionsValidationError } = require("webpack");

const runPrettier = require("../utils/run-prettier");

/**
*
* Runs migration on a given configuration using AST's and promises
* to sequentially transform a configuration file.
*
* @param {String} currentConfigPath - Location of the configuration to be migrated
* @param {String} outputConfigPath - Location to where the configuration should be written
* @param {Object} options - Any additional options regarding code style of the written configuration
*
* Runs migration on a given configuration using AST's and promises
* to sequentially transform a configuration file.
*
* @param {Array} args - Migrate options and arguments such as input and
* output path
* @returns {Promise} Runs the migration using a promise that will throw any errors during each transform
* or output if the user decides to abort the migration
*/

* @returns {Promise} Runs the migration using a promise that will throw any errors during each transform
* or output if the user decides to abort the migration
*/

module.exports = function migrate(
currentConfigPath,
outputConfigPath,
options
) {
module.exports = function migrate(...args) {
const filePaths = args ? args.slice(2).pop() : null;
const currentConfigPath = args.slice(2).length === 1 ? [] : [filePaths];
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed. filePaths will be a string and we can check for that directly. No need to make an array and then using element at 0th index. This will only be useful if we're supporting more than one args.

Copy link
Member Author

Choose a reason for hiding this comment

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

we're supporting more than one args

Copy link
Member

Choose a reason for hiding this comment

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

In migrate? What's the second argument and where is it used in the code?

Copy link
Contributor

@ematipico ematipico Mar 29, 2018

Choose a reason for hiding this comment

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

Better to leave the functionality open to multiple arguments :)
The comments say that the second argument is the output path

if (!filePaths.length) {
Copy link
Member

Choose a reason for hiding this comment

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

(!filePaths || filePaths.length) since filePath can be undefined.
^In case of string, currently it's an array.

Copy link
Member Author

Choose a reason for hiding this comment

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

its a string now, naming is a bit weird

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on line 28, filePaths can be null. There's need for a second check: filePaths.length could throw and error

Copy link
Member

Choose a reason for hiding this comment

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

Bingo! @ematipico Good catch

Copy link
Contributor

Choose a reason for hiding this comment

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

Double check here

if (!filePaths || !filePaths.length)

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha

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 actually tested this manually and we don't need that double check. It's going to be an empty array anyway if filepaths is empty

Copy link
Contributor

Choose a reason for hiding this comment

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

So remove the ternary operator or fall back the assignment to an empty array. It might work but people that see the code might think that is wrong. Or add a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

throw new Error("Please specify a path to your webpack config");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of throwing an error which generates a long non-useful (in this case) stack trace, how about using chalk to show a one-line error message and exit the process gracefully? 😉

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Stack traces are good for us when people submit bugs. Console.errors when we're inside a promise

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks ugly though, error msg is intuitive enough for us to debug

Copy link
Contributor

@ematipico ematipico Apr 2, 2018

Choose a reason for hiding this comment

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

Yeah but in here we are intentionally showing an error to the user, not an error due to a bug in our code. I think we should separate the two concerns

Copy link
Member

@dhruvdutt dhruvdutt Apr 2, 2018

Choose a reason for hiding this comment

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

I'm convinced either way. 😸

}
const outputConfigPath = path.resolve(process.cwd(), filePaths[0]);
Copy link
Member

Choose a reason for hiding this comment

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

No need to create redundancy for config paths if both are same.
We can do just one variable with configPath instead of inputConfigPath and outputConfigPath.

Copy link
Member

Choose a reason for hiding this comment

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

Also, make sure to update this line as well.

fs.readFile(currentConfigPath, "utf8", (err, content) => {

This might have broken the command as in previous code currentConfigPath was a path.resolve and currently, it's an array.

Copy link
Member Author

Choose a reason for hiding this comment

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

see above

const options = {};
const recastOptions = Object.assign(
{
quote: "single"
Expand Down
15 changes: 7 additions & 8 deletions lib/generators/init-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ module.exports = class InitGenerator extends Generator {
constructor(args, opts) {
super(args, opts);
this.isProd = false;
this.dependencies = [
"webpack",
"webpack-cli",
"uglifyjs-webpack-plugin"
];
this.dependencies = ["webpack", "webpack-cli", "uglifyjs-webpack-plugin"];
this.configuration = {
config: {
webpackOptions: {},
Expand Down Expand Up @@ -119,7 +115,9 @@ module.exports = class InitGenerator extends Generator {
Confirm("prodConfirm", "Are you going to use this in production?")
]);
})
.then(prodConfirmAnswer => this.isProd = prodConfirmAnswer["prodConfirm"])
.then(
prodConfirmAnswer => (this.isProd = prodConfirmAnswer["prodConfirm"])
)
.then(() => {
return this.prompt([
Confirm("babelConfirm", "Will you be using ES2015?")
Expand Down Expand Up @@ -398,8 +396,9 @@ module.exports = class InitGenerator extends Generator {
)
])
.then(nameTypeAnswer => {
this.configuration.config.configName = nameTypeAnswer["nameType"].length ?
nameTypeAnswer["nameType"] : defaultName;
this.configuration.config.configName = nameTypeAnswer["nameType"].length
? nameTypeAnswer["nameType"]
: defaultName;
})
.then(() => {
asyncNamePrompt();
Expand Down
3 changes: 2 additions & 1 deletion lib/generators/loader-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function makeLoaderName(name) {
* @class LoaderGenerator
* @extends {Generator}
*/

const LoaderGenerator = webpackGenerator(
[
{
Expand All @@ -36,7 +37,7 @@ const LoaderGenerator = webpackGenerator(
validate: str => str.length > 0
}
],
path.join(__dirname, "templates"),
path.resolve(__dirname, "..", "generate-loader"),
Copy link
Member

Choose a reason for hiding this comment

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

#369 💔

[
"src/cjs.js.tpl",
"test/test-utils.js.tpl",
Expand Down
2 changes: 1 addition & 1 deletion lib/generators/loader-generator.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use strict";

const makeLoaderName = require("./loader-generator").makeLoaderName;
const { makeLoaderName } = require("./loader-generator");
Copy link
Member

Choose a reason for hiding this comment

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

I love these changes ❤️


describe("makeLoaderName", () => {
it("should kebab-case loader name and append '-loader'", () => {
Expand Down
2 changes: 1 addition & 1 deletion lib/generators/plugin-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const PluginGenerator = webpackGenerator(
validate: str => str.length > 0
}
],
path.join(__dirname, "templates"),
path.resolve(__dirname, "..", "generate-plugin"),
[
"src/cjs.js.tpl",
"test/test-utils.js.tpl",
Expand Down
2 changes: 1 addition & 1 deletion lib/generators/webpack-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ function webpackGenerator(
this.npmInstall(["webpack-defaults", "bluebird"], {
"save-dev": true
}).then(() => {
this.spawnCommand("npm", ["run", "webpack-defaults"]);
this.spawnCommand("npm", ["run", "defaults"]);
});
}
};
Expand Down
52 changes: 5 additions & 47 deletions lib/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
"use strict";

const path = require("path");

/**
*
* First function to be called after running a flag. This is a check,
Expand All @@ -14,50 +12,10 @@ const path = require("path");
*/

module.exports = function initialize(command, args) {
const popArgs = args ? args.slice(2).pop() : null;
switch (command) {
case "init": {
const initPkgs = args.slice(2).length === 1 ? [] : [popArgs];
//eslint-disable-next-line
return require("./commands/init.js")(initPkgs);
}
case "migrate": {
const filePaths = args.slice(2).length === 1 ? [] : [popArgs];
if (!filePaths.length) {
throw new Error("Please specify a path to your webpack config");
}
const inputConfigPath = path.resolve(process.cwd(), filePaths[0]);
//eslint-disable-next-line
return require("./commands/migrate.js")(inputConfigPath, inputConfigPath);
}
case "add": {
//eslint-disable-next-line
return require("./commands/add")();
}
case "remove": {
//eslint-disable-next-line
return require("./commands/remove")();
}
case "update": {
return require("./commands/update")();
}
case "serve": {
return require("./commands/serve").serve();
}
case "make": {
return require("./commands/make")();
}
case "generate-loader": {
return require("./generate-loader/index.js")();
}
case "generate-plugin": {
return require("./generate-plugin/index.js")();
}
case "info": {
return require("./commands/info.js")();
}
default: {
throw new Error(`Unknown command ${command} found`);
}
if (!command) {
throw new Error(`Unknown command ${command} found`);
} else if (command === "serve") {
return require(`./commands/${command}`).serve();
Copy link
Member

Choose a reason for hiding this comment

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

Awesome! ❤️

}
return require(`./commands/${command}`)(...args);
Copy link
Contributor

@ematipico ematipico Mar 29, 2018

Choose a reason for hiding this comment

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

This would throw an error no file found. Are we OK with this? Or do we want to throw a better error message?
This is more as a comment, not a blocking change

Copy link
Member

Choose a reason for hiding this comment

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

check for require and then doing this would make sense

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, we've defined all the commands in webpack/bin/webpack.js, rest will the webpack file take care of. What do you mean with check for require?

Copy link
Member

@sendilkumarn sendilkumarn Apr 1, 2018

Choose a reason for hiding this comment

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

Something like if(require(...)) and do check or pass in (..args)

Copy link
Member

Choose a reason for hiding this comment

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

Can't we put this require request in try catch block? and in catch block throw new Error stating that command not found, otherwise the error thrown will be that module was not found.

};
8 changes: 7 additions & 1 deletion lib/init/transformations/top-scope/top-scope.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@

const defineTest = require("../../../utils/defineTest");

defineTest(__dirname, "top-scope", "top-scope-0", ["const test = 'me';"], "init");
defineTest(
__dirname,
"top-scope",
"top-scope-0",
["const test = 'me';"],
"init"
);
defineTest(
__dirname,
"top-scope",
Expand Down
8 changes: 3 additions & 5 deletions lib/migrate/loaderOptionsPlugin/loaderOptionsPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,9 @@ module.exports = function(j, ast) {
// If there is debug: true, set debug: true in the plugin
if (ast.find(j.Identifier, { name: "debug" }).size()) {
loaderOptions.debug = true;
ast
.find(j.Identifier, { name: "debug" })
.forEach(p => {
p.parent.prune();
});
ast.find(j.Identifier, { name: "debug" }).forEach(p => {
p.parent.prune();
});
}

// If there is UglifyJsPlugin, set minimize: true
Expand Down
2 changes: 0 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
"glob-all": "^3.1.0",
"global-modules": "^1.0.0",
"got": "^8.2.0",
"import-local": "^1.0.0",
"inquirer": "^5.1.0",
"interpret": "^1.0.4",
"jscodeshift": "^0.5.0",
Expand All @@ -100,7 +101,6 @@
"p-each-series": "^1.0.0",
"p-lazy": "^1.0.0",
"prettier": "^1.5.3",
"resolve-cwd": "^2.0.0",
"supports-color": "^5.3.0",
"v8-compile-cache": "^1.1.2",
"webpack-addons": "^1.1.5",
Expand Down