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 hot module reloading #175

Merged
merged 1 commit into from
Sep 1, 2020
Merged

Fix hot module reloading #175

merged 1 commit into from
Sep 1, 2020

Conversation

colebemis
Copy link
Contributor

@colebemis colebemis commented Sep 1, 2020

Problem

When making changes to a .md or .mdx page in a Doctocat site, this error appears:

This error occurs because gatsby-plugin-mdx should insert frontmatter in the page context of every page, and does on the first build, but when HMR kicks in the frontmatter is lost.

This is a known bug in gatsby-plugin-mdx: gatsbyjs/gatsby#21837

Possible solutions

  • Solution 1: Explicitly add frontmatter to the page context in our local gatsby-node.js instead of relying on gatsby-plugin-mdx to handle frontmatter for us (This is the solution implemented in this PR)
  • Solution 2: Fix gatsby-plugin-mdx so that it correctly sets the frontmatter when a page is hot reloaded.

Solution 1 (implemented)

@BinaryMuse had previously implemented this solution in #116. However, the GraphQL query that fetched the frontmatter only requested the title field. This caused frontmatter fields like status and source to be lost.

In this PR, I reimplemented @BinaryMuse's solution and added status, source, additionalContributors fields to the frontmatter GraphQL request.

Downsides

  • Adding defaults to destructured variables doesn't work anymore because missing fields are null not undefined. Here's how I worked around that in this PR:
- const {additionalContributors = []} = frontmatter
+ let {additionalContributors} = frontmatter
+ if (!additionalContributors) {
+   additionalContributors = []
+ }
  • Adding extra frontmatter variables won't be added to the page context unless you explicitly add them to the request in gatsby-node.js. This is not obvious and will cause frustration in the future.

Solution 2

I think this is the Right™️ solution, but I'm not sure how to implement it. I would have to a deep dive into the internals of gatsby-plugin-mdx and Gatsby itself.

Closes #146

@vercel
Copy link

vercel bot commented Sep 1, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

doctocat – ./

🔍 Inspect: https://vercel.com/primer/doctocat/l13kbctn4
✅ Preview: https://doctocat-git-fix-hmr.primer.vercel.app

gatsby-theme-primer-example – ./

🔍 Inspect: https://vercel.com/primer/gatsby-theme-primer-example/m265ge3f8
✅ Preview: https://gatsby-theme-primer-example-git-fix-hmr.primer.vercel.app

@colebemis colebemis requested a review from BinaryMuse September 1, 2020 19:33
Copy link
Contributor

@BinaryMuse BinaryMuse left a comment

Choose a reason for hiding this comment

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

👍 I think this looks good to get us going.. if we decide to fix this upstream later we can remove the hack

@colebemis colebemis marked this pull request as ready for review September 1, 2020 20:16
@colebemis colebemis changed the base branch from master to release-0.24.0 September 1, 2020 20:33
@colebemis colebemis merged commit 17e6c49 into release-0.24.0 Sep 1, 2020
This was referenced Sep 1, 2020
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.

Hot reloading breaks for @primer/gatsby-theme-doctocat
2 participants