-
-
Notifications
You must be signed in to change notification settings - Fork 597
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(init): Refactor Yeoman #323
Conversation
CC @addyosmani , can you update your shellJS dependency? They've fixed it I believe shelljs/shelljs#524 (comment) |
lib/generators/init-generator.js
Outdated
@@ -340,7 +340,7 @@ module.exports = class InitGenerator extends Generator { | |||
if (regExpForStyles) { | |||
if (this.isProd) { | |||
this.configuration.config.topScope.push(tooltip.cssPlugin()); | |||
this.dependencies.push("extract-text-webpack-plugin"); | |||
this.dependencies.push("extract-text-webpack-plugin@next"); |
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.
Could add a TODO
here as a reminder to revert it once v4.0.0
is out?
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.
I prefer such hard-coded strings in a variable rather hard-coded. Easier to remember, easier to change and yes for TODO
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.
I think this is pretty intuitive
lib/init/index.js
Outdated
|
||
/** | ||
* | ||
* Runs yeoman and runs the transformations based on the object | ||
* built up from an author/user | ||
* Runs the transformations from an object we get from yeoman |
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.
The comment should be moved after the transformObject
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.
Thanks
return f[0](j, ast, f[1], transformAction); | ||
} | ||
}) | ||
.then(_ => { |
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.
_ => {
can be () => {
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.
Noop functions are _
Google/Chrome convention that is persistent across the source code
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.
But this is not a NOOP function.
Here there is a variable _
that is defined and not used inside the function. This is a NOOP function: _ => {}
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.
yeah it is kinda misleading provided we will also have lodash defined in _
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.
It is rather inconsistent
I think @ev1stensberg is referring to the common practice in functional programming of using _
when the parameter is not used
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.
Ok!
lib/init/index.js
Outdated
return [transformsObject[k]]; | ||
} | ||
}) | ||
.filter(e => e[1]); |
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.
Using e[1]
wouldn't pass the last case of map return [transformsObject[k]]
, but it'll filter with a falsy iteration as e[1]
being undefined
.
Can we clean the else
clause of map
?
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.
It's working as intended
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.
Yeah, It'll get cleared anyways because of falsy iteration. 👍
PTAL |
lib/init/index.js
Outdated
webpackConfig.forEach(scaffoldPiece => { | ||
const config = webpackProperties[scaffoldPiece]; | ||
const transformations = Object.keys(transformsObject) | ||
.map(k => { |
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.
What would be k
a property in the object, specifically a webpackOption
? It would be better to have more descriptive names
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.
Yup, sometimes it is kinda misleading, can we have more descriptive names?
@@ -104,7 +104,7 @@ | |||
"webpack-addons": "^1.1.5", | |||
"yargs": "^11.0.0", | |||
"yeoman-environment": "^2.0.0", | |||
"yeoman-generator": "github:ev1stensberg/generator#Feature-getArgument" | |||
"yeoman-generator": "^2.0.3" |
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.
Nice!
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.
Most awaited. 😅
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.
Also, update the PR description to include "Fixes #161" 🚀
lib/init/index.js
Outdated
}) | ||
.filter(e => e[1]); | ||
const ast = j( | ||
action && action !== "init" |
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 condition is used multiple times. We can DRY the code and move it inside a variable like isActionNotEqualToInit
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.
on it
lib/init/index.js
Outdated
const config = webpackProperties[scaffoldPiece]; | ||
const transformations = Object.keys(transformsObject) | ||
.map(k => { | ||
const stringVal = k.substr(0, k.indexOf("Transform")); |
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.
can we move this into a separate method or a much better way to handle this?
also i think it is better to name it as optionName
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.
On it
env.registerStub(generator, generatorName); | ||
|
||
env.run(generatorName).on("end", () => { | ||
let configModule; | ||
try { | ||
const configPath = path.resolve(process.cwd(), ".yo-rc.json"); |
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.
is the indentation wrong here?
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.
no
lib/utils/modify-config-helper.js
Outdated
// Change structure of the config to be transformed | ||
let tmpConfig = {}; | ||
Object.keys(configModule).forEach(prop => { | ||
const Configs = Object.keys(configModule[prop].configuration); |
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.
configs
?
"\nPlease make sure to use 'this.config.set('configuration', this.configuration);' at the end of the generator.\n" | ||
) | ||
); | ||
Error.stackTraceLimit = 0; |
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.
Doesn't it will set the global error's stackTraceLimit. I would say don't change it or swallow the error here rather than setting something like 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.
It's more intuitive for the user. Run the init command with a yeoman generator not using this.config.set()
(i.e webpack-addons-demo ), and look at the error provided when Stacktrace limit is 0.
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.
I would rather not have a package setting this global configuration.
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.
They're not using other tools, webpack init
is a one-timer
@@ -129,7 +129,7 @@ | |||
"nyc": "^11.4.1", | |||
"prettier-eslint-cli": "^4.7.1", | |||
"schema-utils": "^0.4.5", | |||
"webpack": "^4.0.1", | |||
"webpack": "^4.1.1", |
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.
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.
A rebase will fix it :) not a big problem
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.
LGTM 👍
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.
Please add the TODO
reminding to update extract text plugin once v4 is out,
@ev1stensberg Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
What kind of change does this PR introduce?
Fixes a lot of issues, #161
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
TBD
Summary
Refactors the way generators parse our values to run transforms
For fun
N/A
Does this PR introduce a breaking change?
Yes
Other information