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(www): fail early so it's clearer something is broken! #8700

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/docs/how-to-contribute.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ a pull request.

- Clone the repo and navigate to `/www`
- Run `yarn` to install all of the website's dependencies.
- Create a `.env.development` file with a `GITHUB_API_TOKEN` environment variable. See [www/README][www-readme] for more info!
- Run `gatsby develop` to preview the website in `http://localhost:8000`
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to yarn develop so that it uses the local version of gatsby-cli instead. I was going to do a PR to fix this, but easier to do all at once. :)

- The Markdown files for the documentation live in `/docs` folder. Make
additions or modifications here.
Expand All @@ -91,6 +92,7 @@ To add a new blog post to the gatsbyjs.org blog:

- Clone [the Gatsby repo](https://github.com/gatsbyjs/gatsby/) and navigate to `/www`
- Run `yarn` to install all of the website's dependencies.
- Create a `.env.development` file with a `GITHUB_API_TOKEN` environment variable. See [www/README][www-readme] for more info!
Copy link
Contributor

Choose a reason for hiding this comment

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

It's less welcoming to new contributors to have to go to multiple sources to understand the contributing instructions. Can we make this easier for people by putting all the instructions in one place?

- Run `gatsby develop` to preview the blog at `http://localhost:8000/blog`.
- The content for the blog lives in the `/docs/blog` folder. Make additions or modifications here.
- Add your avatar image to `/docs/blog/avatars`
Expand All @@ -108,3 +110,5 @@ To add a new blog post to the gatsbyjs.org blog:
### Debugging the build process

Check [Debugging the build process](/docs/debugging-the-build-process/) page to learn how to debug Gatsby.

[www-readme]: https://github.com/gatsbyjs/gatsby/tree/master/www#working-with-the-starter-showcase
4 changes: 2 additions & 2 deletions www/gatsby-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ require(`dotenv`).config({
path: `.env.${process.env.NODE_ENV}`,
})

if (process.env.NODE_ENV === `production` && !process.env.GITHUB_API_TOKEN) {
if (!process.env.GITHUB_API_TOKEN) {
throw new Error(
`A GitHub token is required to build the site. Check the README.`
`A GitHub token is required to build the site. Check www/README.md.`
Copy link
Contributor

Choose a reason for hiding this comment

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

@amberleyromo built this so the starter library wouldn’t crash local dev for someone working on something that's not the starter library.

I think that's a good pattern, because getting a GitHub token for an unrelated part of the site you're working on feels like a decent-sized hurdle.

Maybe we can log a better warning about why a GitHub token is required, and let people know they can safely ignore if they're not working on the starter library?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was writing out exactly this comment, thank you @jlengstorf 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlengstorf @amberleyromo this fails for me regardless of which section I'm working on. Does it not for you?

See #5073 (comment)

)
}

Expand Down