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-transformer-remark): properly bubble up errors thrown in subplugins #9972

Merged
merged 17 commits into from
Nov 28, 2018
Merged

fix(gatsby-transformer-remark): properly bubble up errors thrown in subplugins #9972

merged 17 commits into from
Nov 28, 2018

Conversation

tmfelwu
Copy link
Contributor

@tmfelwu tmfelwu commented Nov 16, 2018

This allow gatsby-remark-* subplugins to not have special error handling - gatsby will properly panic on build, but dev server won't crash anymore

@pieh
Copy link
Contributor

pieh commented Nov 16, 2018

This will cause gatsby build to not terminate and might cause broken (but successful) builds. We should wrap katex.renderToString calls in try/catch and use reporter.panicOnBuild (it was fixed some time ago)

@tmfelwu
Copy link
Contributor Author

tmfelwu commented Nov 19, 2018

I didn't check if the build will terminate or not, but i dont think that should happen. anyways if reporter.panicOnBuild is fixed I will use that.

@pieh
Copy link
Contributor

pieh commented Nov 19, 2018

I didn't check if the build will terminate or not, but i dont think that should happen.

Can you elaborate what do You mean? I don't think we want to build be succesful if there are any errors.

@hexrcs hexrcs requested a review from pieh November 27, 2018 16:58
@hexrcs
Copy link
Contributor

hexrcs commented Nov 27, 2018

@sparshy @pieh

Hi guys I tried to make use of reporter.panicOnBuild and added two new test cases, but I wasn't able to get the tests run properly (apparently panicOnBuild quits the test runner process immediately). Would be awesome if someone could help with this and review those changes! 😀

Edit: OK, fixed by using a mock process, now the new unit tests are passing! The lint:other tests failed on packages/gatsby-plugin-google-gtag/README.md, which is unrelated to what this PR is trying to fix. A review would be awesome!

@hexrcs hexrcs requested a review from DSchau November 27, 2018 17:14
@hexrcs
Copy link
Contributor

hexrcs commented Nov 27, 2018

Also it was weird that the linting tests were failing due to some markdown files in other packages 🤔

@hexrcs hexrcs changed the title Fix #6463 Invalid katex leads to server crash fix(gatsby-remark-katex): issue #6463 - Invalid katex leads to server crash Nov 27, 2018
@hexrcs hexrcs changed the title fix(gatsby-remark-katex): issue #6463 - Invalid katex leads to server crash fix(gatsby-remark-katex): invalid katex leads to server crash Nov 27, 2018
@pieh
Copy link
Contributor

pieh commented Nov 27, 2018

You can mock reporter.panicOnBuild instead of mocking process.exit (like in here https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/bootstrap/load-plugins/__tests__/validate.js) and then we only need 1 test (if panicOnBuild was called when there is error)

@hexrcs
Copy link
Contributor

hexrcs commented Nov 27, 2018

@pieh

Cool, didn't think of that! Updated!

@pieh
Copy link
Contributor

pieh commented Nov 28, 2018

I've spend some time local testing it and uncovered that catching this in katex plugin isn't really optimal - we do output error, but because of the caching that we do in gatsby-tranformer-remark, gatsby will actually think that query results are correct and will cache malformatted results.

When I added more error handling in gatsby-trasnformer-remark (instead of katex) I was able to get more meaningful errors (using error handling that we have in gatsby which will report where this is happening - so add more context):

error
The GraphQL query from /Users/misiek/dev/gatsby/examples/using-remark/src/templates/template-blog-post.js failed.

Errors:
  KaTeX parse error: Expected group after '^' at position 14: a^2 + b^2 = d^̲

  GraphQL request (5:5)
  4:   markdownRemark(fields: {slug: {eq: $slug}}) {
  5:     htmlAst
         ^
  6:     timeToRead
  ,KaTeX parse error: Expected group after '^' at position 14: a^2 + b^2 = d^̲

  GraphQL request (6:5)
  5:     htmlAst
  6:     timeToRead
         ^
  7:     tableOfContents
  ,KaTeX parse error: Expected group after '^' at position 14: a^2 + b^2 = d^̲

  GraphQL request (7:5)
  6:     timeToRead
  7:     tableOfContents
         ^
  8:     fields {

URL path:
  /katex/
Context:
  {
    "slug": "/katex/"
  }
Plugin:
  none
Query:
  query usersMisiekDevGatsbyExamplesUsingRemarkSrcTemplatesTemplateBlogPostJs4084396848(
    $slug: String!
  ) {
    markdownRemark(fields: {slug: {eq: $slug}}) {
      htmlAst
      timeToRead
      tableOfContents
      fields {
        tagSlugs
      }
      frontmatter {
        title
        tags
        date(formatString: "MMMM DD, YYYY")
        author {
          id
          bio
          avatar {
            children {
              __typename
              ... on ImageSharp {
                fixed(width: 50, height: 50, quality: 75, grayscale: true) {
                  ...GatsbyImageSharpFixed
                }
              }
            }
          }
        }
      }
    }
  }

  fragment GatsbyImageSharpFixed on ImageSharpFixed {
    base64
    width
    height
    src
    srcSet
  }

I'll clean up my messy code and push it here - sounds good?

@hexrcs
Copy link
Contributor

hexrcs commented Nov 28, 2018

@pieh That would be awesome!

@pieh pieh changed the title fix(gatsby-remark-katex): invalid katex leads to server crash fix(gatsby-transformer-remark): properly bubble up errors thrown in subplugins Nov 28, 2018
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @sparshy and @hexrcs for help here!

@pieh pieh merged commit b7d4656 into gatsbyjs:master Nov 28, 2018
@gatsbot
Copy link

gatsbot bot commented Nov 28, 2018

Holy buckets, @sparshy — 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. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  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!

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…ubplugins (gatsbyjs#9972)

This allow `gatsby-remark-*` subplugins to not have special error handling - `gatsby` will properly panic on build, but dev server won't crash anymore

<!--
  Q. Which branch should I use for my pull request?
  A. Use `master` branch (probably).

  Q. Which branch if my change is a bug fix for Gatsby v1?
  A. In this case, you should use the `v1` branch

  Q. Which branch if I'm still not sure?
  A. Use `master` branch. Ask in the PR if you're not sure and a Gatsby maintainer will be happy to help :)

  Note: We will only accept bug fixes for Gatsby v1. New features should be added to Gatsby v2.

  Learn more about contributing: https://www.gatsbyjs.org/docs/how-to-contribute/
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants