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
5 changes: 4 additions & 1 deletion lib/generators/init-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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


if (cssBundleName.length !== 0) {
this.configuration.config.webpackOptions.plugins.push(
Expand Down Expand Up @@ -410,4 +410,7 @@ module.exports = class InitGenerator extends Generator {
});
});
}
writing() {
this.config.set("configuration", this.configuration);
}
};
187 changes: 137 additions & 50 deletions lib/init/index.js
Original file line number Diff line number Diff line change
@@ -1,65 +1,152 @@
"use strict";

const yeoman = require("yeoman-environment");
const Generator = require("yeoman-generator");
const path = require("path");
const defaultGenerator = require("../generators/init-generator");
const runTransform = require("./transformations/index");
const j = require("jscodeshift");
const chalk = require("chalk");
const pEachSeries = require("p-each-series");

const runPrettier = require("../utils/run-prettier");

const entryTransform = require("./transformations/entry/entry");
const outputTransform = require("./transformations/output/output");
const contextTransform = require("./transformations/context/context");
const resolveTransform = require("./transformations/resolve/resolve");
const devtoolTransform = require("./transformations/devtool/devtool");
const targetTransform = require("./transformations/target/target");
const watchTransform = require("./transformations/watch/watch");
const watchOptionsTransform = require("./transformations/watch/watchOptions");
const externalsTransform = require("./transformations/externals/externals");
const nodeTransform = require("./transformations/node/node");
const performanceTransform = require("./transformations/performance/performance");
const statsTransform = require("./transformations/stats/stats");
const amdTransform = require("./transformations/other/amd");
const bailTransform = require("./transformations/other/bail");
const cacheTransform = require("./transformations/other/cache");
const profileTransform = require("./transformations/other/profile");
const mergeTransform = require("./transformations/other/merge");
const parallelismTransform = require("./transformations/other/parallelism");
const recordsInputPathTransform = require("./transformations/other/recordsInputPath");
const recordsOutputPathTransform = require("./transformations/other/recordsOutputPath");
const recordsPathTransform = require("./transformations/other/recordsPath");
const moduleTransform = require("./transformations/module/module");
const pluginsTransform = require("./transformations/plugins/plugins");
const topScopeTransform = require("./transformations/top-scope/top-scope");
const devServerTransform = require("./transformations/devServer/devServer");
const modeTransform = require("./transformations/mode/mode");
const resolveLoaderTransform = require("./transformations/resolveLoader/resolveLoader");

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

*
* @param {String} options - An path to the given generator
* @returns {Function} runTransform - Run transformations based on the finished
* yeoman instance
* @param {Object} webpackProperties - Configuration to transform
* @param {String} action - Action to be done on the given ast
* @returns {Promise} - A promise that writes each transform, runs prettier
* and writes the file
*/

function creator(options) {
let env = yeoman.createEnv("webpack", null);
const generatorName = options
? replaceGeneratorName(path.basename(options[0]))
: "webpack-default-generator";
if (options) {
const WebpackGenerator = class extends Generator {
initializing() {
options.forEach(path => {
return this.composeWith(require.resolve(path));
});
}
};
env.registerStub(WebpackGenerator, generatorName);
} else {
env.registerStub(defaultGenerator, "webpack-default-generator");
}
const transformsObject = {
entryTransform,
outputTransform,
contextTransform,
resolveTransform,
devtoolTransform,
targetTransform,
watchTransform,
watchOptionsTransform,
externalsTransform,
nodeTransform,
performanceTransform,
statsTransform,
amdTransform,
bailTransform,
cacheTransform,
profileTransform,
moduleTransform,
pluginsTransform,
topScopeTransform,
mergeTransform,
devServerTransform,
modeTransform,
parallelismTransform,
recordsInputPathTransform,
recordsOutputPathTransform,
recordsPathTransform,
resolveLoaderTransform
};

env.run(generatorName).on("end", _ => {
if (generatorName !== "webpack-default-generator") {
//HACK / FIXME
env = env.options.env;
return runTransform(env.configuration);
} else {
return runTransform(env.getArgument("configuration"));
}
module.exports = function runTransform(webpackProperties, action) {
// webpackOptions.name sent to nameTransform if match
const webpackConfig = Object.keys(webpackProperties).filter(p => {
return p !== "configFile" && p !== "configPath";
});
}
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?

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

if (Object.keys(config.webpackOptions).length) {
if (config.webpackOptions[stringVal]) {
return [transformsObject[k], config.webpackOptions[stringVal]];
} else {
return [transformsObject[k], config[stringVal]];
}
} else {
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. 👍

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

? webpackProperties.configFile
: "module.exports = {}"
);
const transformAction = action || null;

/*
* @function replaceGeneratorName
*
* Replaces the webpack-addons pattern with the end of the addons name merged
* with 'generator'
*
* @param { String } name - name of the generator
* @returns { String } name - replaced pattern of the name
*/
return pEachSeries(transformations, f => {
if (!f[1]) {
return f[0](j, ast, transformAction);
} else {
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!

let configurationName;
if (!config.configName) {
configurationName = "webpack.config.js";
} else {
configurationName = "webpack." + config.configName + ".js";
}

function replaceGeneratorName(name) {
return name.replace(/(webpack-addons)?([^:]+)(:.*)?/g, "generator$2");
}
const outputPath =
action && action !== "init"
? webpackProperties.configPath
: path.join(process.cwd(), configurationName);
const source = ast.toSource({
quote: "single"
});

module.exports = {
creator,
replaceGeneratorName
runPrettier(outputPath, source);
})
.catch(err => {
console.error(err.message ? err.message : err);
});
});
if (action === "add" && webpackProperties.config.item) {
process.stdout.write(
"\n" +
chalk.green(
`Congratulations! ${
webpackProperties.config.item
} has been ${action}ed!\n`
)
);
} else {
process.stdout.write(
"\n" +
chalk.green(
"Congratulations! Your new webpack configuration file has been created!\n"
)
);
}
};
10 changes: 0 additions & 10 deletions lib/init/index.test.js

This file was deleted.

153 changes: 0 additions & 153 deletions lib/init/transformations/index.js

This file was deleted.

Loading