-
-
Notifications
You must be signed in to change notification settings - Fork 603
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 4 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,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"); | ||||
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. 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]; | ||||
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 is not needed. 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. we're supporting more than one 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. 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. Better to leave the functionality open to multiple arguments :) |
||||
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 outputConfigPath = path.resolve(process.cwd(), filePaths[0]); | ||||
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 need to create redundancy for config paths if both are same. 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. Also, make sure to update this line as well. webpack-cli/lib/commands/migrate.js Line 45 in 0de8a4e
This might have broken the command as in previous code 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. see above |
||||
const options = {}; | ||||
const recastOptions = Object.assign( | ||||
{ | ||||
quote: "single" | ||||
|
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 |
||
}; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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?