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: include npm install for site subdirectory #1531

Merged
merged 1 commit into from
Nov 10, 2020
Merged

docs: include npm install for site subdirectory #1531

merged 1 commit into from
Nov 10, 2020

Conversation

evans
Copy link
Contributor

@evans evans commented Nov 7, 2020

- Summary

I got unresolved dependencies errors during the pre-push hooks, since I hadn't installed the dependencies in the site subdirectory. This PR adds instructions to install those missing deps to the contributing guide. If you think there's a better place or alternative, happy to change the PR or you're welcome to take over!

Screen Shot 2020-11-06 at 8 33 20 PM

- Test plan

I've tested locally, though I wish there was a way to test/maintain the contributor onboarding experience. If there's something I'm not aware of, I'm down to look at the implementation.

- Description for the changelog

docs: add instructions to install npm dependencies in the site subdirectory

- A picture of a cute animal (not mandatory but encouraged)

༼ つ ◕_◕ ༽つ🤳

@evans evans requested a review from a team as a code owner November 7, 2020 04:37
Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Thanks @evans for your contribution!

There is a npm run site:build task that could be used instead:

"site:build": "run-s site:build:*",

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label Nov 7, 2020
@evans evans requested a review from ehmicky November 9, 2020 05:39
@evans
Copy link
Contributor Author

evans commented Nov 9, 2020

Thanks @ehmicky!

I thought about adding something similar to the readme's development section, though that seems to be aimed at how to get up and running quickly

CONTRIBUTING.md Outdated
@@ -22,7 +22,7 @@ First fork and clone the repository. If you're not sure how to do this, please w
Run:

```bash
npm install
npm install && npm run site:build
Copy link
Contributor

Choose a reason for hiding this comment

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

We just add a discussion with @erezrokah and were thinking npm run site:build:install might actually make more sense, since the linting errors are only due to uninstalled dependencies. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense 👍 I'll make the update

Originally I was hesitant to use npm ci when reading into site:build:install. Now I think it's a reasonable decision over npm install, since it'll stay out of the way, installing stable dependencies. I'm assuming there's more value to smoothing the starting path than staying compatibility with the state of the user's site/node_modules, since I'm guessing they're coming to the contributing getting started as a landing page or while debugging. Also a vast majority of contributions are probably made to the main project, so I figure it's a non-issue and changes made to site will be done by someone familiar with the setup.

Unverified

This user has not yet uploaded their public signing key.
@evans evans requested a review from ehmicky November 10, 2020 05:11
@ehmicky
Copy link
Contributor

ehmicky commented Nov 10, 2020

Thanks @evans!

@ehmicky ehmicky merged commit be88233 into netlify:master Nov 10, 2020
@evans evans deleted the evans/add-site-install-docs branch November 10, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants