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
16 changes: 9 additions & 7 deletions bin/webpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@
(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;
}

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"]
})
);
};
11 changes: 7 additions & 4 deletions lib/commands/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@ 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) {
if (pkg.length === 0) {
module.exports = function initializeInquirer(...args) {
const packages = args ? args.slice(3) : null;

if (packages.length === 0) {
return modifyHelper("init", defaultGenerator);
}
return npmPackagesExists(pkg);
return npmPackagesExists(packages);
};
89 changes: 64 additions & 25 deletions lib/commands/migrate.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,81 @@
"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 arguments such as input and
* output path
* @returns {Function} Runs the migration using the 'runMigrate'
* function.
*/

* @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(...args) {
const filePaths = args ? args.slice(3) : null;
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 currentConfigPath = path.resolve(process.cwd(), filePaths[0]);
let outputConfigPath;
if (!filePaths[1]) {
return inquirer
.prompt([
{
type: "confirm",
name: "confirmPath",
message:
"Migration output path not specified. " +
"Do you want to use your existing webpack " +
"configuration?",
default: "Y"
}
])
.then(ans => {
if (!ans["confirmPath"]) {
console.log(chalk.red("✖ ︎Migration aborted due no output path"));
process.exitCode = 1;
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 without return, this might continue with migration.

Copy link
Member Author

Choose a reason for hiding this comment

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

no

Copy link
Member

Choose a reason for hiding this comment

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

screenshot_20180401_165304

Copy link
Member Author

Choose a reason for hiding this comment

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

Migration aborted due no output path

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@dhruvdutt dhruvdutt Apr 1, 2018

Choose a reason for hiding this comment

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

That's true but exitCode won't exit the process as it doesn't call the exit() method implicitly. To exit the process we'll need to do:

process.exitCode = 1;
process.exit(); // this will exit with exit code 1

or

process.exit(1); // this will also exit with exit code 1

or add a return;.

Without exiting, the process will continue and do the migration even after the chalk error log.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dhruvdutt is right, we need to abort from the chain of promises. But return won't help. I'd suggest to throw an error and handle it inside the catch

}
outputConfigPath = path.resolve(process.cwd(), filePaths[0]);
return runMigration(currentConfigPath, outputConfigPath);
})
.catch(err => {
console.error(err);
});
}
outputConfigPath = path.resolve(process.cwd(), filePaths[1]);
return runMigration(currentConfigPath, outputConfigPath);
};

module.exports = function migrate(
currentConfigPath,
outputConfigPath,
options
) {
const recastOptions = Object.assign(
{
quote: "single"
},
options
);
/**
*
* Runs migration on a given configuration using AST's and promises
* to sequentially transform a configuration file.
*
* @param {String} currentConfigPath - input path for config
* @param {String} outputConfigPath - output path for config
* @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
*/

function runMigration(currentConfigPath, outputConfigPath) {
const recastOptions = {
quote: "single"
};
const tasks = new Listr([
{
title: "Reading webpack config",
Expand Down Expand Up @@ -149,4 +188,4 @@ module.exports = function migrate(
console.error(err);
process.exitCode = 1;
});
};
}
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
Loading