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

Vite: Support legacyMdx1 fallback flag #20823

Merged
merged 8 commits into from
Feb 13, 2023
Merged

Vite: Support legacyMdx1 fallback flag #20823

merged 8 commits into from
Feb 13, 2023

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Jan 30, 2023

Issue: #

What I did

Pair to #20747, adds support for legacyMdx1 feature flag to vite projects.

How to test

Same as webpack projects listed in the PR above. Note I didn't actually test it myself.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@@ -17,6 +17,7 @@
- [Babel mode v7 exclusively](#babel-mode-v7-exclusively)
- [Importing plain markdown files with `transcludeMarkdown` has changed](#importing-plain-markdown-files-with-transcludemarkdown-has-changed)
- [7.0 feature flags removed](#70-feature-flags-removed)
- [Story context is prepared before for supporting fine grained updates](#story-context-is-prepared-before-for-supporting-fine-grained-updates)
Copy link
Member Author

Choose a reason for hiding this comment

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

This did not have a TOC before my vscode addon picked it up and added it. But I think this change isn't the most general for all users, so I moved it from the top to lower on the list. I honestly have a bit of a hard time understanding what users need to do based on the migration instructions, but maybe others do.

IanVS added 2 commits February 6, 2023 07:30
# Conflicts:
#	code/lib/builder-vite/src/plugins/mdx-plugin.ts
#	code/yarn.lock
@IanVS
Copy link
Member Author

IanVS commented Feb 6, 2023

@valentinpalkovic I've fixed the conflicts here. Let me know if you have any concerns, it would be great to get this in soon, to achieve parity with the Webpack flag.

@valentinpalkovic
Copy link
Contributor

@IanVS I followed the test instructions in the other PR for a vite sandbox. I have adjusted the name of Introduction.mdx to Introduction.stories.mdx. I have added MDX1 only syntax and enabled the legacy mdx1 feature flag. Unfortunately, the Introduction site is blank in Storybook.

@shilman shilman changed the title Vite: support legacyMdx1 fallback flag Vite: Support legacyMdx1 fallback flag Feb 6, 2023
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@IanVS
Copy link
Member Author

IanVS commented Feb 6, 2023

Note I didn't actually test it myself.

Guess I should have. :(

I did things the same way as webpack, so I'm confused why it's not working, but I'll take a look. Thanks.

@IanVS
Copy link
Member Author

IanVS commented Feb 6, 2023

I'm seeing that the mdx1-csf transform is adding pragmas of '/* @jsxRuntime classic */\n' and '/* @jsx mdx */\n', and then using mdx as a global like:

    ' return mdx(MDXLayout, _extends({}, layoutProps, props, {\n' +
    '    components: components,\n' +
    '    mdxType: "MDXLayout"\n' +
    '  }), mdx(Meta, {\n' +
    '    title: "Example/Introduction",\n' +
    '    mdxType: "Meta"\n' +
    '  }), mdx("style", null, `\n' +

Which results in a ReferenceError: mdx is not defined error.

@IanVS
Copy link
Member Author

IanVS commented Feb 7, 2023

OK, I figured this out. the MDX1 compiler for Webpack uses a loader which injects the mdx compiler (https://github.com/storybookjs/mdx1-csf/blob/d58cb032a8902b3f24ad487b6a7aae11ba8b33f6/loader.js#L12-L16), which we used to do in the vite plugin too, but removed for mdx2 because it was no longer necessary. So, this re-introduces the injecting of the mdx compiler, being sure to grab it from the right version of @mdx-js/react.

@shilman
Copy link
Member

shilman commented Feb 9, 2023

@IanVS does the fix of adding the MDX compiler belong in the mdx1-csf package? Ideally mdx1-csf and mdx2-csf should be interchangeable and it looks like they're not. I must've forgotten about that change in mdx2-csf. Or maybe I'm misunderstanding.

@IanVS
Copy link
Member Author

IanVS commented Feb 9, 2023

I think ideally you're right, adding the mdx import should probably be done in the compile function itself, and not in the loader. That would allow them to be swapped out interchangeably (more or less, I think). I'm not sure if it was a change that we actually had to make in mdx2-csf, or mdx just compiles differently now and doesn't require the same kind of addition.

IanVS added a commit to storybookjs/builder-vite that referenced this pull request Feb 10, 2023
fixes #557

I found that mdx was not working correctly here, and @valentinpalkovic realized my fallback in Storybook 7 wasn't either (storybookjs/storybook#20823).  It was because we used to inject the mdx compiler into the source code that we get back from the `@storybook/mdx1-csf` compiler, but we lost that in https://github.com/storybookjs/builder-vite/pull/548/files#diff-f2dfc4dfa9074d77a728a88e6629a1d66be8a0765cab8562526cd00fbae910e5L6.  This re-introduces it, with a bit better of a guarantee that the correct version is going to be loaded, by starting from inside `@storybook/mdx1-csf`.

I've also pinned mdx1-csf here, in case the import is moved from the loader to the compiler, as @shilman has suggested doing.  We'll need to adjust this if that happens.
It is handled internally to mdx1-csf now.
@storybookjs storybookjs deleted a comment from socket-security bot Feb 13, 2023
@shilman shilman merged commit 2897b98 into next Feb 13, 2023
@shilman shilman deleted the vite/mdx1 branch February 13, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants