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

docs: init documentaion #547

Merged
merged 5 commits into from Jul 19, 2018
Merged

docs: init documentaion #547

merged 5 commits into from Jul 19, 2018

Conversation

rishabh3112
Copy link
Member

What kind of change does this PR introduce?

documentation of init feature
Did you add tests for your changes?
no
If relevant, did you update the documentation?
yes
Does this PR introduce a breaking change?

no
Other information
Refers #247

Add setup walk through and add usage description
Add setup walk through and add usage description
@jsf-clabot
Copy link

jsf-clabot commented Jul 16, 2018

CLA assistant check
All committers have signed the CLA.

INIT.md Outdated
1. Create `package.json` through npm

```shell
$ npm init
Copy link
Member

Choose a reason for hiding this comment

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

Trim the whitespace in left.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Never seen shell markdown. Is it an alias of bash?

Copy link
Member Author

Choose a reason for hiding this comment

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

shell used in github flavoured markdown alias to bash in common markdown

Copy link
Member

Choose a reason for hiding this comment

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

shell, bash, sh are just kinda labels. None reflects anything in syntax highlighting.

INIT.md Outdated
$ npm install --save-dev webpack webpack-cli
```

3. Optional step : Install @webpack-cli/init package to add init addon
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be optional I guess because this document is exclusive for the init command.

Copy link
Member Author

Choose a reason for hiding this comment

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

When webpack-cli init is runned it install the package on its own.
Given step 2 is followed

Copy link
Contributor

@ematipico ematipico Jul 16, 2018

Choose a reason for hiding this comment

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

I think it should be mandatory, not optional

INIT.md Outdated
...
"scripts": {
...
"project-init" : "webpack-cli init",
Copy link
Member

Choose a reason for hiding this comment

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

No need for this step. We can directly do webpack-cli init

Copy link
Member Author

Choose a reason for hiding this comment

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

We cant do that directly if webpack-cli isnt installed globally.
I mean wasnt when I unlinked the github repo and installed from npm

Copy link
Contributor

Choose a reason for hiding this comment

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

dhruvdutt is right, there's no need of the npm script. If the user followed up to the second step, they can simply run the cli command through

npx webpack-cli init

If the package is installed locally, or

webpack-cli init

If the package is installed globally. I think we should make this distinction during all the steps, otherwise the users gets confused

Copy link
Member Author

Choose a reason for hiding this comment

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

Does npx comes up with npm? as i think it is a package that needed to be installed using npm itself, increasing dependency. maybe?!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes npx comes with npm out of the box! It has no deps and it won't be part of the package.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! just discovered and is coming since npm 5.2!!
So, should we also add up global setup @ematipico ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say so! So we'd cover all cases and we clear up everything!

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.

Seems like @ematipico & @dhruvdutt got their hands on this one :)

@webpack-bot
Copy link

Hi @rishabh3112.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own master branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

Help Needed: setting up webpack-cli globally
@rishabh3112
Copy link
Member Author

Please verify global setup @dhruvdutt

INIT.md Outdated
Follow following steps to setup `webpack-cli init` globally:
1. Install `webpack` and `webpack-cli` globally
```shell
$ sudo npm install -g webpack webpack-cli
Copy link
Member

Choose a reason for hiding this comment

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

Why doing sudo? Global modules are installed at user level, not root.

If you get errors in permission if you remove sudo then you probably have installed node/npm incorrectly in your local machine. Find the fix on Google. For now, remove sudo from here. :)

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 was getting errors,
Will remove sudo from both steps in next commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dhruvdutt , also I wasn't able to install webpack-cli globally using npm but webpack was installed.
Added this because it is only method (i.e. using -g flag) makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

What error are you getting?

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 running this:
$ sudo npm install -g webpack-cli

npm WARN checkPermissions Missing write access to /usr/lib/node_modules/webpack-cli
npm WARN webpack-cli@3.0.8 requires a peer of webpack@^4.x.x but none is installed. You must install peer dependencies yourself.

I have webpack@4.16.1
But was able to setup webpack-command

Copy link
Member

Choose a reason for hiding this comment

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

This issue is not related to webpack-cli. It happens because you've installed node/npm with incorrect permission. This is a common issue which I've also experienced it. You can Google it and find a fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was able to fix it by setting it up using nvm .

INIT.md Outdated
$ webpack-cli init
```

### Questions asked by generator
Copy link
Member

Choose a reason for hiding this comment

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

Nice work. 💯 🥇

How about mentioning which specific property/key the question is related to and also provide a link to webpack docs for every question? Similar to how you did for entry. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will where ever possible in next commit.

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 has been resolved @dhruvdutt ?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Why have we skipped mentioning property for the last question?

INIT.md Outdated
$ npm install --save-dev webpack webpack-cli
```

3. Install @webpack-cli/init package to add init addon
Copy link
Member

Choose a reason for hiding this comment

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

@webpack-cli/init => @webpack-cli/init

INIT.md Outdated
$ sudo npm install -g webpack webpack-cli
```

2. Install @webpack-cli/init package to add init addon
Copy link
Member

Choose a reason for hiding this comment

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

@webpack-cli/init => @webpack-cli/init

Add Property/keys section under each question and other minor changes
@webpack-bot
Copy link

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

@rishabh3112
Copy link
Member Author

Any Changes Required @ematipico

2. Install `webpack` and `webpack-cli` as devDependencies

```shell
$ npm install --save-dev webpack webpack-cli
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be consistent and would write --save-dev or -D, not both. They are basically the same. -D is just an alias

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it @ematipico

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.

Just a really minor change, but overall it looks good to me!

Change -D flag to --save-dev flag
@webpack-bot
Copy link

@rishabh3112 Thanks for your update.

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

@ematipico Please review the new changes.

@dhruvdutt dhruvdutt merged commit 3b40708 into webpack:master Jul 19, 2018
@dhruvdutt
Copy link
Member

@rishabh3112 Awesome work. Cheers and thank you for being so thorough. 💯 ❤️

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