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(init): Refactor Yeoman #323

Merged
merged 11 commits into from
Mar 9, 2018
Merged

cli(init): Refactor Yeoman #323

merged 11 commits into from
Mar 9, 2018

Conversation

evenstensberg
Copy link
Member

@evenstensberg evenstensberg commented Mar 8, 2018

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

@evenstensberg
Copy link
Member Author

CC @addyosmani , can you update your shellJS dependency? They've fixed it I believe shelljs/shelljs#524 (comment)

@@ -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");
Copy link
Contributor

@ematipico ematipico Mar 8, 2018

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?

Copy link
Contributor

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

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 think this is pretty intuitive


/**
*
* 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
Copy link
Contributor

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

Copy link
Member Author

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(_ => {
Copy link
Contributor

Choose a reason for hiding this comment

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

_ => { can be () => {

Copy link
Member Author

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

Copy link
Contributor

@ematipico ematipico Mar 9, 2018

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: _ => {}

Copy link
Member

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 _

Copy link
Member

Choose a reason for hiding this comment

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

It is rather inconsistent
image
I think @ev1stensberg is referring to the common practice in functional programming of using _ when the parameter is not used

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok!

return [transformsObject[k]];
}
})
.filter(e => e[1]);
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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. 👍

@evenstensberg
Copy link
Member Author

PTAL

webpackConfig.forEach(scaffoldPiece => {
const config = webpackProperties[scaffoldPiece];
const transformations = Object.keys(transformsObject)
.map(k => {
Copy link
Member

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

Copy link
Member

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"
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Most awaited. 😅

Copy link
Member

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" 🚀

})
.filter(e => e[1]);
const ast = j(
action && action !== "init"
Copy link
Contributor

@ematipico ematipico Mar 9, 2018

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

Copy link
Member Author

Choose a reason for hiding this comment

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

on it

const config = webpackProperties[scaffoldPiece];
const transformations = Object.keys(transformsObject)
.map(k => {
const stringVal = k.substr(0, k.indexOf("Transform"));
Copy link
Member

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

Copy link
Member Author

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");
Copy link
Member

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?

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

// Change structure of the config to be transformed
let tmpConfig = {};
Object.keys(configModule).forEach(prop => {
const Configs = Object.keys(configModule[prop].configuration);
Copy link
Member

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;
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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",
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 going to conflict with #319.

Copy link
Contributor

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

Copy link
Member

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@ematipico ematipico left a 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,

@webpack-bot
Copy link

@ev1stensberg Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants