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(v2): move metadata export after compiling MDX to avoid weird MDX parsing error. #2105

Merged
merged 5 commits into from
Dec 11, 2019

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented Dec 9, 2019

Motivation

Move metadata export after compiling the MDX to avoid weird MDX parsing error.
Although we've fixed #2095 (MDX thinking that metadata was part of paragraph), there might be some other unexpected case.

For example:

This will error out, because it is unclosed (funnily it thinks its unclosed because style must always have </style> counterpart. And MDX think metadata is part of the JSX

## Hello World
<style dangerouslySetInnerHTML={{__html: `
  .comparisonTable td {
    width: 25%;
  }
  .comparisonTable .middle {
    text-align: center;
    vertical-align: middle;
  }
  .comparisonTable ul {
    padding-left: 1em;
  }
`}}/>

export const metadata = {
  id: 'test'
}

https://mdxjs.com/playground
image

If its closed properly, it doesnt error. Now I'm pretty confused how they determine HTML or JSX

## Hello World
<style dangerouslySetInnerHTML={{__html: `
  .comparisonTable td {
    width: 25%;
  }
  .comparisonTable .middle {
    text-align: center;
    vertical-align: middle;
  }
  .comparisonTable ul {
    padding-left: 1em;
  }
`}}></style>

export const metadata = {
  id: 'test'
}

image

A real scenario is https://github.com/CanopyTax/single-spa.js.org/blob/master/docs/separating-applications.md
image

Now, we will just do the export on top level on mdx-loader part. See code in this PR packages/docusaurus-mdx-loader/src/index.js

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

Test with this markdown. No more error.
image

image

Hot reload still working and everything

hot reload still fine for metadata

@endiliey endiliey added the pr: bug fix This PR fixes a bug in a past release. label Dec 9, 2019
@endiliey endiliey requested a review from yangshun as a code owner December 9, 2019 12:22
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 9, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 8d92f08

https://deploy-preview-2105--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 8d92f08

https://deploy-preview-2105--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit d74a94e

https://deploy-preview-2105--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit d74a94e

https://deploy-preview-2105--docusaurus-preview.netlify.com

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Will all of our markdown/MDX files have front matter? I think it's a requirement for us but not part of the spec. Wonder if this breaks for users who have markdown files which don't have front matter.

@endiliey
Copy link
Contributor Author

No it doesnt need to have frontmatter. Actually Ive always tested docusaurus 2 building 1000 files without any frontmatter at all. It also wont break if it doesnt have frontmatter. You can give it a try

Btw the frontMatter export code was written by you 😂

@endiliey
Copy link
Contributor Author

frontMatter export PR here #1450

Authored by @yangshun

@yangshun
Copy link
Contributor

yangshun commented Dec 11, 2019 via email

@endiliey endiliey merged commit ace93c5 into master Dec 11, 2019
@endiliey endiliey deleted the endi/metadata-better branch December 11, 2019 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants