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

Improve migration docs for MDX2 markdown import #19733

Closed
mihkeleidast opened this issue Nov 2, 2022 · 6 comments · Fixed by #20796
Closed

Improve migration docs for MDX2 markdown import #19733

mihkeleidast opened this issue Nov 2, 2022 · 6 comments · Fixed by #20796
Assignees

Comments

@mihkeleidast
Copy link
Contributor

mihkeleidast commented Nov 2, 2022

Describe the bug

Importing a markdown file (changelog) that includes some "special syntax" causes the mdx page to not render.

Minimal markdown:

# Changelog

- <IconSort/> prop definitions

Co-authored-by: Foo Bar <foo@google.com>

The two issues in there are:

  • <IconSort/> is not wrapped in backticks, so mdx2 treats it as a component, but does not find the import
  • It does not like the email syntax <foo@google.com>
    • Error in browser: Failed to fetch dynamically imported module: http://localhost:9001/src/docs/changelog.stories.mdx?t=1667071749836&import
    • Error in console:
    10:29:17 PM [vite] Internal server error: Unexpected character `@` (U+0040) in name, expected a name character such as letters, digits, `$`, or `_`; whitespace before attributes; or the end of the tag (note: to create a link in MDX, use `[text](url)`)
 Plugin: storybook:mdx-plugin
 File: /foo/bar/CHANGELOG.md:undefined:undefined

To Reproduce

No response

System

No response

Additional context

Works without errors with SB6.5 and mdx1

@shilman
Copy link
Member

shilman commented Nov 3, 2022

@JReinhold would love to brainstorm about this one. the issue is that MDX2 has gotten a lot more strict about markdown syntax than MDX1, so things that "transcluded" properly don't anymore.

We could either:

  1. Explain to users in the migration notes
  2. Figure out a different way to import and render markdown in MDX

@JReinhold
Copy link
Contributor

I have a strong preference for option 1., otherwise it feels like we're making our own flavor of MDX2, and that's just an API surface that's really difficult to support down the line.

But we can talk this over in more detail.

@mihkeleidast
Copy link
Contributor Author

I would have no objections if this was mostly a migration docs fix - I had only three offending lines, so not hard to fix. However, it would be very nice if the error message could be improved so it would point to the offending place in the markdown file, manually trying to figure out what is wrong in a file thousands of lines long is a bit of a hassle.

@shilman shilman removed this from MDX2 Dec 12, 2022
@shilman shilman moved this to Required for RC in Core Team Projects Dec 12, 2022
@tmeasday
Copy link
Member

@shilman shall we change the title of this one to essentially be 1?

@shilman shilman added documentation and removed bug labels Jan 11, 2023
@shilman shilman changed the title [Bug]: 7 alpha, mdx2, external markdown import Improve migration docs for MDX2 markdown import Jan 11, 2023
@shilman
Copy link
Member

shilman commented Jan 23, 2023

Here's the plan:

@shilman shilman moved this from Required for RC to Required for QA in Core Team Projects Jan 23, 2023
@JReinhold JReinhold moved this from Required for QA to In Progress in Core Team Projects Jan 25, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Core Team Projects Jan 27, 2023
@shilman
Copy link
Member

shilman commented Jan 28, 2023

Zoinks!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.36 containing PR #20796 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants