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

fix(www): fail early so it's clearer something is broken! #8700

Closed
wants to merge 4 commits into from
Closed

fix(www): fail early so it's clearer something is broken! #8700

wants to merge 4 commits into from

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Oct 2, 2018 •
edited
Loading

Related to #5073

This fails earlier (i.e. in dev/prod/any environment) if the necessary environment variable doesn't exist.

@DSchau DSchau requested a review from amberleyromo October 2, 2018 14:38
@DSchau DSchau requested a review from a team as a code owner October 2, 2018 14:38
@DSchau DSchau requested a review from a team October 2, 2018 14:41
@DSchau
Copy link
Contributor Author

DSchau commented Oct 2, 2018

cc @LekoArts

LekoArts
LekoArts reviewed Oct 2, 2018
View reviewed changes
www/gatsby-node.js Outdated
@@ -14,7 +14,7 @@ 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.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe www/README to make it more clear?

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're in www, I don't know if this is clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But sure! Not too opinionated either way 👍

docs/docs/how-to-contribute.md Outdated
@@ -91,6 +91,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.md](https://github.com/gatsbyjs/gatsby/tree/master/www#working-with-the-starter-showcase) 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.

I'd also add that to the documentation part above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need reusable fragments 🙃 😄

LekoArts
LekoArts approved these changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

👍

jlengstorf
jlengstorf suggested changes Oct 2, 2018
View reviewed changes
Copy link
Contributor

@jlengstorf jlengstorf left a comment

Choose a reason for hiding this comment

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

I'm not sure the state here is actually broken. Tagging in @amberleyromo since she created this workflow.

www/gatsby-node.js
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)

siakaramalegos
siakaramalegos reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@siakaramalegos siakaramalegos left a comment

Choose a reason for hiding this comment

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

Just wanted to provide some suggestions, but don't want it to block anything.

docs/docs/how-to-contribute.md
@@ -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. :)

docs/docs/how-to-contribute.md
@@ -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?

@amberleyromo
Copy link
Contributor

@LekoArts adding the token solved your issue? #8693 (comment)

What I would like to fix here is why lacking the token caused the issue you reported in the other PR. It should just quietly set default empty values for the missing Github info

@DSchau
Copy link
Contributor Author

DSchau commented Oct 2, 2018

@amberleyromo @jlengstorf @LekoArts let's close this in favor of fixing the underlying issue (e.g. default to null/empty fields without a Github token) rather than adding extra hoops :)

@DSchau DSchau closed this Oct 2, 2018
@DSchau DSchau deleted the www/env-var-fail-early branch October 2, 2018 15:00
@LekoArts
Copy link
Contributor

LekoArts commented Oct 2, 2018

@amberleyromo Actually I doesn't work with the .env.development in place. Same error. I just assumed that the test of @DSchau solved it 🤔

@DSchau
Copy link
Contributor Author

DSchau commented Oct 2, 2018

@LekoArts heh, it worked for me 🙃 You get the same error?

@amberleyromo
Copy link
Contributor

@LekoArts I'll look into why this is failing for you. If you figure out why it's not working, PR welcome! :)

@LekoArts
Copy link
Contributor

LekoArts commented Oct 2, 2018

@DSchau @amberleyromo Deleted my .cache folder and now it's working (with the .env.development file.

@amberleyromo amberleyromo mentioned this pull request Oct 2, 2018
Merged
DSchau pushed a commit that referenced this pull request Oct 2, 2018
@amberleyromo @DSchau
[www/starters] tweaks to fix DX for no GH token on www (#8718)
7bfb4b0 
Relevant to #8700 

The mocked defaults for when there's no token / no github data fetched were incorrectly formatted. This fix ensures that you can develop on `www` without a token.

cc @DSchau
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siakaramalegos siakaramalegos siakaramalegos left review comments

@jlengstorf jlengstorf jlengstorf requested changes

@LekoArts LekoArts LekoArts approved these changes

@amberleyromo amberleyromo Awaiting requested review from amberleyromo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DSchau @amberleyromo @LekoArts @jlengstorf @siakaramalegos