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

feat: switch to xdm #9

Merged
merged 3 commits into from
Mar 13, 2021
Merged

feat: switch to xdm #9

merged 3 commits into from
Mar 13, 2021

Conversation

kentcdodds
Copy link
Owner

@kentcdodds kentcdodds commented Mar 13, 2021

What: switch to xdm

Why: It's better than mdx-js

How: use dynamic imports which is how you maintain your own CJS-ness but import native ESMs. This is desirable for now.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

@kentcdodds
Copy link
Owner Author

cc @Arcath and @wooorm

@kentcdodds
Copy link
Owner Author

kentcdodds commented Mar 13, 2021

Two issues:

Looks like xdm is considering all import syntax to actually just be strings. Not sure what's going on there...

image

Jest can't update the snapshots because the filename is "<stdin>" somehow 🙃

image

Other than this, I think syntactically this works ok and should work for folks using mdx-bundler!

src/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@Arcath Arcath mentioned this pull request Mar 13, 2021
3 tasks
kentcdodds and others added 2 commits March 13, 2021 15:39
Co-authored-by: Titus <tituswormer@gmail.com>
@kentcdodds
Copy link
Owner Author

I couldn't figure out what was up with the <stdin> thing in Jest, but honestly I don't really care all that much. I'm tired of fighting this and hope that eventually it'll just fix itself. This is ready to rock and roll.

@kentcdodds
Copy link
Owner Author

Oh, and I tested this with remix and it totes works (when remix isn't compiling away the dynamic import, this has been reported).

@kentcdodds kentcdodds merged commit c49e165 into main Mar 13, 2021
@kentcdodds kentcdodds deleted the pr/xdm branch March 13, 2021 22:53
@kentcdodds
Copy link
Owner Author

@all-contributors please add @wooorm for code

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @wooorm! 🎉

@github-actions
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Arcath
Copy link
Collaborator

Arcath commented Mar 13, 2021

@wooorm Using a providerImportSource of @mdx-js/react doesn't appear to be working as I'd expect it to from the docs.

I'm displaying it with this: (where components is an object of replacements).

   const Component = useMemo(() => getMDXComponent(source), [source])

  return <MDXProvider components={components}>
    <Component />
  </MDXProvider>

Is bundling or the way the client creates the component causing some issues here?

For completeness my bundleMDX is:

const {code} = await bundleMDX(source, {
    files,
    xdmOptions: (input, options) => {
      options.remarkPlugins = [
        remarkHighlight
      ]

      options.providerImportSource = '@mdx-js/react'

      return options
    }
  })

@kentcdodds
Copy link
Owner Author

If I understand it correctly, I think you could side-step the @mdx-js/react package entirely with:

   const Component = useMemo(() => getMDXComponent(source), [source])

  return <Component components={components} />

@wooorm
Copy link

wooorm commented Mar 14, 2021

@kentcdodds is correct here: in most cases, you don’t need to provider. I prefer being explicit and passing those components.

Still, it should be possible to use a provider. “Is bundling or the way the client creates the component causing some issues here?” I think so. I’m guessing that esbuild is bundling @mdx-js/react, which creates a context. And the client is using another @mdx-js/react, so with a different context?

@Arcath
Copy link
Collaborator

Arcath commented Mar 14, 2021

@kentcdodds yep that works. Its a TypeScript issue as components is not there as a prop in in the return type.

@kentcdodds
Copy link
Owner Author

Yeah, I realized that yesterday as well. Need to fix the typings

@Arcath
Copy link
Collaborator

Arcath commented Mar 14, 2021

#12 fixes the typings

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

Successfully merging this pull request may close these issues.

None yet

3 participants