cli(init): Refactor Yeoman #323

evenstensberg · 2018-03-08T18:10:10Z

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 · 2018-03-08T18:14:45Z

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

ematipico · 2018-03-08T20:54:27Z

lib/generators/init-generator.js

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


Could add a TODO here as a reminder to revert it once v4.0.0 is out?

I prefer such hard-coded strings in a variable rather hard-coded. Easier to remember, easier to change and yes for TODO

I think this is pretty intuitive

ematipico · 2018-03-08T20:56:12Z

lib/init/index.js


 /**
 *
- * 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


The comment should be moved after the transformObject

ematipico · 2018-03-08T20:58:51Z

lib/init/index.js

+				return f[0](j, ast, f[1], transformAction);
+			}
+		})
+			.then(_ => {


_ => { can be () => {

Noop functions are _ Google/Chrome convention that is persistent across the source code

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

yeah it is kinda misleading provided we will also have lodash defined in _

It is rather inconsistent

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

dhruvdutt · 2018-03-08T22:49:31Z

lib/init/index.js

+					return [transformsObject[k]];
+				}
+			})
+			.filter(e => e[1]);


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?

It's working as intended

Yeah, It'll get cleared anyways because of falsy iteration. 👍

evenstensberg · 2018-03-08T23:34:09Z

PTAL

montogeek · 2018-03-09T05:41:28Z

lib/init/index.js

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


What would be k a property in the object, specifically a webpackOption? It would be better to have more descriptive names

Yup, sometimes it is kinda misleading, can we have more descriptive names?

montogeek · 2018-03-09T05:42:09Z

package.json

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


Most awaited. 😅

Also, update the PR description to include "Fixes #161" 🚀

ematipico · 2018-03-09T05:59:38Z

lib/init/index.js

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


This condition is used multiple times. We can DRY the code and move it inside a variable like isActionNotEqualToInit

sendilkumarn · 2018-03-09T05:59:08Z

lib/init/index.js

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


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

sendilkumarn · 2018-03-09T06:15:45Z

lib/utils/modify-config-helper.js

 	env.registerStub(generator, generatorName);

 	env.run(generatorName).on("end", () => {
+		let configModule;
+		try {
+			const configPath = path.resolve(process.cwd(), ".yo-rc.json");


is the indentation wrong here?

sendilkumarn · 2018-03-09T06:16:51Z

lib/utils/modify-config-helper.js

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


sendilkumarn · 2018-03-09T06:18:25Z

lib/utils/modify-config-helper.js

+					"\nPlease make sure to use 'this.config.set('configuration', this.configuration);' at the end of the generator.\n"
+				)
+			);
+			Error.stackTraceLimit = 0;


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.

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.

I would rather not have a package setting this global configuration.

They're not using other tools, webpack init is a one-timer

dhruvdutt · 2018-03-09T09:09:59Z

package.json

@@ -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",


This is going to conflict with #319.

A rebase will fix it :) not a big problem

dhruvdutt

LGTM 👍

ematipico

Please add the TODO reminding to update extract text plugin once v4 is out,

webpack-bot · 2018-03-09T17:50:26Z

@ev1stensberg Thanks for your update.

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

@ematipico Please review the new changes.

webpack-bot · 2018-03-09T18:30:33Z

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

evenstensberg added 4 commits March 8, 2018 16:56

misc(refactor): refactoring work on init

5a4e3e1

chore(rebase): rebase to master

5c0b7c1

misc(refactor): refactor init

6969f5f

misc(refactor): refactor init

77f8f53

webpack-bot added the PR: unreviewed label Mar 8, 2018

evenstensberg requested review from ematipico and fokusferit March 8, 2018 18:11

evenstensberg mentioned this pull request Mar 8, 2018

fix(generators) Use webpack 4 version of extract-text-webpack-plugin #322

Closed

chore(deps): remove redundant dep

ee66351

This was referenced Mar 8, 2018

Webpack init trying to use unsupported extract-text-webpack-plugin #320

Closed

Required git? #318

Closed

webpack-bot added the PR: CI-ok label Mar 8, 2018

ematipico reviewed Mar 8, 2018

View reviewed changes

dhruvdutt reviewed Mar 8, 2018

View reviewed changes

misc(jsdocs): change occuring comment

363d29a

montogeek reviewed Mar 9, 2018

View reviewed changes

ematipico reviewed Mar 9, 2018

View reviewed changes

sendilkumarn requested changes Mar 9, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Mar 9, 2018

dhruvdutt reviewed Mar 9, 2018

View reviewed changes

andrewpmontgomery mentioned this pull request Mar 9, 2018

Cannot install webpack-cli via npm #326

Closed

dhruvdutt mentioned this pull request Mar 9, 2018

Remove local fork of yeoman #161

Closed

cli(init): move transform method to func

7fee593

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Mar 9, 2018

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Mar 9, 2018

misc(appveyor): only report bundlesize in travis

9fc6c52

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Mar 9, 2018

dhruvdutt approved these changes Mar 9, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Mar 9, 2018

ematipico suggested changes Mar 9, 2018

View reviewed changes

webpack-bot added the PR: reviewed-changes-requested label Mar 9, 2018

misc(comment): add comment to indicate future replacement

5d19f45

webpack-bot added PR: review-outdated and removed PR: reviewed-approved PR: reviewed-changes-requested labels Mar 9, 2018

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Mar 9, 2018

misc(lint): fix linting errors

5aaac64

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Mar 9, 2018

evenstensberg mentioned this pull request Mar 9, 2018

yeoman-generator #329

Closed

evenstensberg merged commit 6e9cb5f into master Mar 9, 2018

evenstensberg deleted the yeoman-templates branch March 9, 2018 23:38

dhruvdutt mentioned this pull request Mar 15, 2018

cli(add): write configuration to yeoman file #348

Merged

cli(init): Refactor Yeoman #323

cli(init): Refactor Yeoman #323

Conversation

evenstensberg commented Mar 8, 2018 • edited Loading

evenstensberg commented Mar 8, 2018

ematipico Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvdutt left a comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

webpack-bot commented Mar 9, 2018

webpack-bot commented Mar 9, 2018

evenstensberg commented Mar 8, 2018 •

edited

Loading

ematipico Mar 8, 2018 •

edited

Loading

ematipico Mar 9, 2018 •

edited

Loading

ematipico Mar 9, 2018 •

edited

Loading