-
-
Notifications
You must be signed in to change notification settings - Fork 621
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
Changes from 6 commits
dc71c53
8e786fe
20f15b0
ed679cf
c4bb9f9
e3c8fd8
d0e6bdb
5721513
80161f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"); | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its a string now, naming is a bit weird There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on line 28, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bingo! @ematipico Good catch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double check here if (!filePaths || !filePaths.length) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
throw new Error("Please specify a path to your webpack config"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true but 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 Without exiting, the process will continue and do the migration even after the chalk error log. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
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", | ||
|
@@ -149,4 +188,4 @@ module.exports = function migrate( | |
console.error(err); | ||
process.exitCode = 1; | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ function makeLoaderName(name) { | |
* @class LoaderGenerator | ||
* @extends {Generator} | ||
*/ | ||
|
||
const LoaderGenerator = webpackGenerator( | ||
[ | ||
{ | ||
|
@@ -36,7 +37,7 @@ const LoaderGenerator = webpackGenerator( | |
validate: str => str.length > 0 | ||
} | ||
], | ||
path.join(__dirname, "templates"), | ||
path.resolve(__dirname, "..", "generate-loader"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #369 💔 |
||
[ | ||
"src/cjs.js.tpl", | ||
"test/test-utils.js.tpl", | ||
|
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'", () => { | ||
|
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, | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome! ❤️ |
||
} | ||
return require(`./commands/${command}`)(...args); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, we've defined all the commands in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we put 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.
nit: can we merge this together?