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.

Sign up for GitHub

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

Jump to bottom

docs: fix the setup workflow #591 #597

Merged
merged 4 commits into from
Sep 18, 2018
Merged

docs: fix the setup workflow #591 #597

merged 4 commits into from
Sep 18, 2018

Conversation

EvsChen
Copy link
Contributor

@EvsChen EvsChen commented Sep 17, 2018

What kind of change does this PR introduce?
Improve docs

Summary
As mentioned in #591 , the setup guidance is not clear enough for newcomers who setup the directory for the first time. The bootstrap command is necessary.

Does this PR introduce a breaking change?
No

@jsf-clabot
Copy link

jsf-clabot commented Sep 17, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@webpack-bot
Copy link

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

evenstensberg
evenstensberg requested changes Sep 17, 2018
View reviewed changes
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.

A minor comment and it looks good to me 🙏

CONTRIBUTING.md Outdated
@@ -34,6 +34,7 @@ In case you are suggesting a new feature, we will match your idea with our curre

### Setup with npm
* Install the dependencies: `npm install`
* Bootstrap the packages:`npx lerna bootstrap`
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 to use npx, regular npm is sufficient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean npm lerna bootstrap? It doesn't seem to work for me. I use [email protected]

Copy link
Member

Choose a reason for hiding this comment

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

Npm run bootstrap is the command you’re looking for I assume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. My initial thought was to keep consistent between yarn and npm.

@webpack-bot
Copy link

@EvsChen Thanks for your update.

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

@ev1stensberg Please review the new changes.

evenstensberg
evenstensberg reviewed Sep 17, 2018
View reviewed changes
CONTRIBUTING.md Outdated
@@ -42,6 +43,7 @@ In case you are suggesting a new feature, we will match your idea with our curre

```bash
yarn install
yarn lerna bootstrap
Copy link
Member

Choose a reason for hiding this comment

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

Yarn bootstrap

evenstensberg
evenstensberg requested changes Sep 17, 2018
View reviewed changes
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.

Its yarn bootstrap that needs change, keep it as the old commit, but change the lerna instruction to be correct

evenstensberg
evenstensberg requested changes Sep 17, 2018
View reviewed changes
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.

Keep the install commands and it looks good

@EvsChen
Copy link
Contributor Author

EvsChen commented Sep 17, 2018

Sorry I'm a little confused. I think it's either npm run bootstrap or npm install && npx lerna bootstrap, since the bootstrap command runs clean all. The same for yarn.

@evenstensberg
Copy link
Member

You need to install deps first. Add those lines and youre good. Npm install and yarn

@EvsChen
Copy link
Contributor Author

EvsChen commented Sep 17, 2018

But there is a npm install in the bootstrap script.

evenstensberg
evenstensberg approved these changes Sep 17, 2018
View reviewed changes
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

@EvsChen
Copy link
Contributor Author

EvsChen commented Sep 17, 2018

Thanks for your patience. Sorry I didn't make my point clear.

@ematipico ematipico merged commit 2910645 into webpack:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico approved these changes

@dhruvdutt dhruvdutt dhruvdutt approved these changes

@rishabh3112 rishabh3112 rishabh3112 approved these changes

@evenstensberg evenstensberg evenstensberg approved these changes

Assignees
No one assigned
Labels
PR: CI-ok PR: reviewed-approved PR: reviewed-changes-requested PR: small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@EvsChen @jsf-clabot @webpack-bot @evenstensberg @ematipico @dhruvdutt @rishabh3112