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(gatsby): Panic when root config is a function #16272

Conversation

@cephalization
Copy link
Contributor

commented Jul 31, 2019

Not 100% sure if or where tests for this should be written, I could not find any other tests for bootstrap's index. I did use gatsby-dev to test that this change works however and it appears to do so.

Additionally, not sure if we want this panic to occur on configs that are still using experimental configs AND non experimental configs so it does it before loading either config.

Description

When bootstrap loads the root config, throw a panic if the config is of type Function.

Related Issues

Fixes #16265

@cephalization cephalization requested a review from gatsbyjs/themes-core as a code owner Jul 31, 2019

@ChristopherBiscardi
Copy link
Member

left a comment

Just the more expanded error message would be great. This way users new to Gatsby have a little more context.

@ChristopherBiscardi ChristopherBiscardi requested a review from gatsbyjs/core Jul 31, 2019

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Could you convert the error to be a structured error? E.g. #15619

We're switching all errors over and we shouldn't add new non-structured errors.

To convert it, you need to add an error entry to https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-cli/src/structured-errors/error-map.js and then reference that in the error object you pass into report.panic.

Thanks! Can answer any questions you have. We need to create a doc page for this as well.

@cephalization

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Could you convert the error to be a structured error? E.g. #15619

We're switching all errors over and we shouldn't add new non-structured errors.

To convert it, you need to add an error entry to https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-cli/src/structured-errors/error-map.js and then reference that in the error object you pass into report.panic.

Thanks! Can answer any questions you have. We need to create a doc page for this as well.

I can give it a shot 👍

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@cephalization

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Does the id of the structured error just iterate off of the last error id? I see now that the errors are partitioned with incremented IDs

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

The errors are pretty random actually :-D

Part of the goal of structured errors is that people can google the numbers and find something specific to the error. So it doesn't really matter what you pick as long as it isn't the same as another error and it isn't a "common" number e.g. 123456.

Incremented IDs tend to come up when someone is doing a PR converted unstructured errors and converts several together. There's no meaning otherwise.

@cephalization

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

2019-07-31 20_12_59-gatsby-config js - gatsby-test - Visual Studio Code

@cephalization

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Not sure why circle is choking, will look into it tomorrow

@cephalization cephalization force-pushed the cephalization:themes/panic-on-function-gatsby-config branch from 116d8ba to 5139917 Aug 1, 2019

Improve error messaging around gatsby-config
- Includes and displays file path when root gatsby-config is exported as
a function
@cephalization

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Screen Shot 2019-08-01 at 6 42 48 AM

@sidharthachatterjee sidharthachatterjee changed the title Panic when root config is a function fix(gatsby): Panic when root config is a function Aug 1, 2019

@sidharthachatterjee
Copy link
Member

left a comment

This is great! Thank you so much @cephalization 👍

Also, congratulations on your first contribution to Gatsby 💃

@sidharthachatterjee sidharthachatterjee merged commit 81ff489 into gatsbyjs:master Aug 1, 2019

18 checks passed

Danger All good
Details
Peril All green. Congrats.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
unit_tests_windows Build #20190801.10 succeeded
Details
@gatsbot

This comment has been minimized.

Copy link

commented Aug 1, 2019

Holy buckets, @cephalization — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

johno added a commit to johno/gatsby that referenced this pull request Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.