Refactor lib/creator and integrate validation in migrate by evenstensberg · Pull Request #99 · webpack/webpack-cli

evenstensberg · 2017-04-05T17:36:01Z

Did you add tests for your changes?
N/A

If relevant, did you update the documentation?

N/A

Summary

Lays the groundwork for a first release. Integrated Validation to
migrate, refactored creator for the init feature to not be circular and
made a cleaner structure to the project inside lib/creator.

Does this PR introduce a breaking change?
No

Other information

Lays the groundwork for a first release. Integrated Validation to migrate, refactored creator for the init feature to not be circular and made a cleaner structure to the project inside lib/creator.

evenstensberg · 2017-04-05T17:40:35Z

@okonet Better to open this in PR in a text editor to see the changes. Could you review once you find the time?

okonet · 2017-04-06T07:44:54Z

+module.exports = function (promptOptions) {
+	const promptParams = Object.keys(promptOptions);
+	if (promptParams && promptParams.length) {
+		let ast = j('{}');


That's not the Object as you might expect. This is a block statement. See https://astexplorer.net/#/gist/8ae05b2650bc27d2c4923ef8e6f36241/0a32afa94ee7cd5ecd742958c815706d34201345

okonet · 2017-04-06T07:46:39Z

+				.forEach(
+					p => p.value.properties.push(
+						j.property('init', j.literal('output'),
+						j.objectExpression([j.property('init', j.literal('filename'), j.literal(value))])))


You should check out the utils we've written for migrate feature: https://github.com/webpack/webpack-cli/blob/master/lib/transformations/utils.js#L82

okonet · 2017-04-06T07:47:29Z

-						fs.writeFileSync(outputConfigPath, result, 'utf8');
-						console.log(chalk.green(`✔︎ New webpack v2 config file is at ${outputConfigPath}`));
+						fs.writeFile(outputConfigPath, result, 'utf8', (err) => {
+							const webpackOptionsValidationErrors = validateSchema(webpackOptionsSchema, require(outputConfigPath));


Should this go to a separate PR?

Probably not needed. Fixing the transform issues and let's merge and take it from there.

I'm working on a fix PR and desperately need this! :)

evenstensberg · 2017-04-06T13:29:26Z

@pksjce I've started the transformations, but let's move it to another seperate branch after this gets merged. I'll open a wip for you to follow along.

eliperelman · 2017-04-06T22:48:34Z

+	const env = yeoman.createEnv(null, null, new WebpackAdapter());
+
+	if(options) {
+		env.register(require.resolve(options), 'npm:app');


I'd probably rename npm:app to avoid any potential collisions by other packages that may have registered a Yeoman environment. Maybe webpack:cli?

It's gonna be assigned to the generator name :) See previous comment.

eliperelman · 2017-04-06T22:49:49Z

+		env.register(require.resolve(options), 'npm:app');
+
+		env.run('npm:app')
+		.on('end', () => {


This is an arrow function, so you could either just one-line it and leave out the return, or since it's an event, you probably don't need the return at all.

This is actually just the first integration, would need to change the generatorName through env.getGeneratorMetaData() later, could integrate it now though.

okonet

LGTM in general. See a couple of comments

okonet · 2017-04-07T08:01:01Z

+*/
+
+function replaceGeneratorName(name) {
+	return name.replace(


Mind adding tests for this one?

Sure, giveme 1h :) At school

okonet · 2017-04-07T08:02:39Z

+		env.register(require.resolve(options), generatorName);
+
+		env.run(generatorName)
+		.on('end', () => {


Can the duplication be removed from these 2 code branches?

Also indentation is odd but this can be fixed as #102 is merged,

Yeah, I was thinking about it actually, will do that.

pksjce · 2017-04-07T08:12:28Z

Cool. I will work on a separate PR for transformations. My previous PR was meant for feedback. I was really not sure what approach would work with yeoman.
Also could you solve these
https://www.codacy.com/app/okonet/webpack-cli/pullRequest?prid=594576
Would be good to comment them out for now.

Also commented out unused modules for now.

evenstensberg · 2017-04-07T08:34:49Z

There we go @okonet @pksjce . I've started the work on the AST's @pksjce , lemme open a wip with my stuff for us to work with.

codecov · 2017-04-07T11:36:41Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6e93af5). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #99   +/-   ##
=========================================
  Coverage          ?   94.49%           
=========================================
  Files             ?       13           
  Lines             ?      363           
  Branches          ?       75           
=========================================
  Hits              ?      343           
  Misses            ?       20           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e93af5...717b61e. Read the comment docs.

okonet · 2017-04-07T11:37:33Z

@@ -0,0 +1,15 @@
+'use strict';
+
+function replaceGeneratorName(name) {


Should this be the real implementation? Export it and import here to remove code duplication, please.

okonet · 2017-04-07T11:40:20Z

+
+	it('should replace a pattern of an addon', () => {
+		const generatorName = replaceGeneratorName('webpack-addons-thefox');
+		expect(generatorName).toEqual('generator-thefox');


The regex looks more complex as just this test and covers more edge cases. Should we test against them?

Just an initial commit, can add more tests later and label an issue with good first contribution

feat: refactor

77dc227

Lays the groundwork for a first release. Integrated Validation to migrate, refactored creator for the init feature to not be circular and made a cleaner structure to the project inside lib/creator.

evenstensberg added CLI Init enhancement AST Transform v1 labels Apr 5, 2017

evenstensberg added this to the v1 milestone Apr 5, 2017

evenstensberg requested a review from okonet April 5, 2017 17:36

This was referenced Apr 5, 2017

Enhancements: Refactor logic in lib/creator #98

Closed

Feature [--migrate]: Validate the generated webpack 2 configuration. #48

Closed

evenstensberg requested a review from pksjce April 5, 2017 18:37

okonet reviewed Apr 6, 2017

View reviewed changes

evenstensberg force-pushed the feature/refactor branch from 9581031 to 77dc227 Compare April 6, 2017 13:26

eliperelman reviewed Apr 6, 2017

View reviewed changes

feat: Use custom generatorNames

9a84b36

evenstensberg force-pushed the feature/refactor branch from fbf8392 to 9a84b36 Compare April 7, 2017 07:48

okonet reviewed Apr 7, 2017

View reviewed changes

feat: Simply logic for yeoman

8870ec0

Also commented out unused modules for now.

okonet approved these changes Apr 7, 2017

View reviewed changes

fix: add tests

3dee6de

evenstensberg force-pushed the feature/refactor branch from 3833e75 to 3dee6de Compare April 7, 2017 11:33

Merge master into feature/refactor

bd98877

okonet suggested changes Apr 7, 2017

View reviewed changes

okonet reviewed Apr 7, 2017

View reviewed changes

fix: use real implementation for test

717b61e

okonet approved these changes Apr 7, 2017

View reviewed changes

evenstensberg merged commit 66280b9 into master Apr 7, 2017

evenstensberg deleted the feature/refactor branch April 7, 2017 11:51

		@@ -0,0 +1,15 @@
		'use strict';

		function replaceGeneratorName(name) {

Uh oh!

Uh oh!

Conversation

evenstensberg commented Apr 5, 2017

Uh oh!

evenstensberg commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okonet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pksjce commented Apr 7, 2017

Uh oh!

evenstensberg commented Apr 7, 2017

Uh oh!

codecov Bot commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okonet Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Apr 7, 2017 •

edited

Loading

okonet Apr 7, 2017 •

edited

Loading