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

fix(generator): using configFile in configPath to get the config file name #883

Merged
merged 10 commits into from May 24, 2019
Merged

fix(generator): using configFile in configPath to get the config file name #883

merged 10 commits into from May 24, 2019

Conversation

anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented May 12, 2019

Passing {configFile:configFile} to the env.run(generator) as second
argument in packages/utils/modify-config-helper.ts and using this property in generator class to get the
configPath in generator class constructor in remove-generator.ts

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
No

If relevant, did you update the documentation?
Not required

Summary
Issue link : #882
In order to avoid the typeError error while running the remove command when passing the config file name other than webpack.config.js
Added a configFile property while running the generator from modify-config-helper.ts :

	env.registerStub(generator, generatorName);

	env.run(generatorName,{
		configFile
	}).then()

In order to pass the user's config file name to the generator class and replace the hardcoded webpack.config.js to this

	const {configFile} = opts
		let configPath = path.resolve(process.cwd(), configFile);
		const webpackConfigExists = fs.existsSync(configPath);

Does this PR introduce a breaking change?

No

Other information
No need of this statement I think

		if (!webpackConfigExists) {
			configPath = null;
			// end the generator stating webpack config not found or to specify the config
		}

Cause the below statement in modify-config-helper.ts will take care of that

	if (webpackConfigExists) {
			process.stdout.write(
				"\n" +
					logSymbols.success +
					chalk.green(" SUCCESS ") +
					"Found config " +
					chalk.cyan(configFile + "\n") +
					"\n"
			);

		} else {
			process.stdout.write(
				"\n" +
					logSymbols.error +
					chalk.red(" ERROR ") +
					chalk.cyan(configFile) +
					" not found. Please specify a valid path to your webpack config like \n " +
					chalk.white("$ ") +
					chalk.cyan(`webpack-cli ${action} webpack.dev.js`) +
					"\n"
			);
			return;
		}

Passing {configFile:configFile} to the env.run(generator) as second
argument in packages/utils/modify-config.ts and using this property in generator class to get the
configPath in generator class constructor in remove-generator.ts
@webpack-bot
Copy link

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

const webpackConfigExists = fs.existsSync(configPath);

if (!webpackConfigExists) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This checking is not required now. what do you think @ematipico ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@misterdev what about this one ? should I remove this if statement?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you are right! We can remove it

Copy link
Contributor

@misterdev misterdev left a comment

Choose a reason for hiding this comment

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

Hi, seems ok to me, I've left some comments about formatting

Thank you! :)

@@ -32,13 +32,15 @@ export default class RemoveGenerator extends Generator {
webpackOptions: {}
}
};

let configPath = path.resolve(process.cwd(), "webpack.config.js");
const {configFile} = opts
Copy link
Contributor

Choose a reason for hiding this comment

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

Add spaces and ;
const { configFile } = opts;

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

@@ -58,13 +58,14 @@ export default function modifyHelperUtil(
chalk.cyan(configFile + "\n") +
"\n"
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why this empty line? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

removed it 👍

@@ -89,8 +90,9 @@ export default function modifyHelperUtil(
}
env.registerStub(generator, generatorName);

env.run(generatorName)
.then(
env.run(generatorName,{
Copy link
Contributor

Choose a reason for hiding this comment

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

Add space

env.run(generatorName, {

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

added semicolon in line 35
removed the blank line and added space in env.run method 's second argument.
@anikethsaha
Copy link
Member Author

last two commits, 0b65ba8 and e007c45 are chore not fix , my bad :)

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.

Some feedback

@@ -32,13 +32,15 @@ export default class RemoveGenerator extends Generator {
webpackOptions: {}
}
};

let configPath = path.resolve(process.cwd(), "webpack.config.js");
const {configFile} = opts;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const {configFile} = opts;
const { configFile } = opts;

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

const webpackConfigExists = fs.existsSync(configPath);

if (!webpackConfigExists) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you are right! We can remove it

env.run(generatorName)
.then(
env.run(generatorName, {
configFile
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this change for?

Copy link
Member Author

Choose a reason for hiding this comment

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

To pass the config file name to the generator.

Copy link
Member

Choose a reason for hiding this comment

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

This change is probably ok and needed as we need this to be dynamic. Just gotta check that it works. Change is that the generator will have output of the config file supplied vs webpack.config.js

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I used this in my machine and its working fine. This can use workable with the other generators too like the update one and in add too

removed the if-statement and added space
@anikethsaha
Copy link
Member Author

@ematipico Please review it again, I made those clean ups

Changed the init-generator code to fix the entry point issue when the
entry point was answered
Copy link
Contributor

@misterdev misterdev 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
Member

@anshumanv anshumanv left a comment

Choose a reason for hiding this comment

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

Neat ✨

@ematipico
Copy link
Contributor

Please rebase, we removed node 6 from the CI

@webpack-bot
Copy link

@anikethsaha Thanks for your update.

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

@anshumanv Please review the new changes.

@anikethsaha
Copy link
Member Author

PTAL @ematipico

@ematipico ematipico merged commit fe31e65 into webpack:master May 24, 2019
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.

None yet

6 participants