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

feat: install init as global dep #630

Merged
merged 3 commits into from
Nov 4, 2018
Merged

Conversation

manu-chroma
Copy link
Contributor

Closes #564

I am a little confused on how to test this change. Like how to reference my local webpack-cli installation in some other test folder where I can invoke the init command

@jsf-clabot
Copy link

jsf-clabot commented Oct 7, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

You'll need to edit the dep lookup too, I fear that it might be the case that we just install it globally, but it doesn't pick it up. Try to have 2 cases, one to resolve the global another local. If the global fails, go for local. If both fails, prompt for a global installation.

@@ -58,6 +58,15 @@ module.exports = function promptForInstallation(packages, ...args) {
options[0] = "add";
}

if (packages === "init") {
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to make this more embeded instead of splicing. Maybe make a new object instead.

@evenstensberg
Copy link
Member

Also saw this was your first PR, congratulations! 💙

@manu-chroma
Copy link
Contributor Author

Thanks @ev1stensberg 🥂

Which file should I search for the dep lookup?

@webpack-bot
Copy link

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

@evenstensberg
Copy link
Member

Its the require resolve in this file, but invoking a addon might also be tested, that logic is under the init pacakge. I advice you to experiment a bit yourself, add some test commands like npm link to ensure that it works

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

lgtm :shipit:

@evenstensberg
Copy link
Member

Thanks for fixing this @manu-chroma 💙

@manu-chroma
Copy link
Contributor Author

Hey, thanks for finishing this up. Apologies for not completing this one myself, I'll make sure that I do, in the future! 😃

@evenstensberg evenstensberg merged commit d8a121a into webpack:master Nov 4, 2018
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.

4 participants