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

Add run, runSync exports #1792

Merged
merged 1 commit into from
Nov 13, 2021
Merged

Add run, runSync exports #1792

merged 1 commit into from
Nov 13, 2021

Conversation

yordis
Copy link
Contributor

@yordis yordis commented Nov 3, 2021

Can you expand on the problem you’re having

I was replacing mdx-bundler, where I used the following feature:

import { getMDXComponent } from 'mdx-bundler/client';

In the setup I have the following routing in NextJS:

<root>/src/pages/adrs/[slug].tsx

Notice the [slug], which means that part of the routing is dynamic. I am doing
the following in my getStaticProps:

// server-side NextJS
export const getStaticProps: GetStaticProps<SlugProps, RouteParam> = async (props) => {
  const source = await readFile(`${props.params!.slug}/index.mdx`, {
    // Eventually this become a configuration, the intention is that you can
    // have something like https://docusaurus.io/ tool
    atRootDir: '@/routes/adrs/routes/[slug]/routes', 
  });
  const post = await mdx.compile(source, {
    outputFormat: 'function-body', // notice the usage of function-body
    // ...
  });
  
  return {
    props: {
      post: {
        code: post.value,
      },
    },
  };
};

So then in the client, I can use props.post.code eventually in my client.

import { getMDXModule } from '@mdx-js/react';

export function MDX(props: MDXProps) {
  const Component = React.useMemo(() => getMDXModule(props.source), [props.source]);
  return <Component.default components={props.slots} />;
}

export function Slug(props: SlugProps) {
  return <MDX source={props.post.code} />;
}

How this solves that, and whether there are potential alternatives?

I am not sure if there is a better alternative and way to do this.

The directory containing the ADRs will be outside my app, so I am not sure
if I should use import(...) directly from the client side. How does the file system lookup would work? I am not sure

Happy to hear from alternatives, the end-goal is to allow people to use such
setup for documentation like docusaurus but specific to ADRs and the tool.

Should this feature be here? In this package? Could it be somewhere else? Where?

Since everything is related to MDX and React, it feels the right place. And
it feels that mdx@v2 was definitely taking into consideration this since at least I found function-body in it that feels it could be used for use cases
(I would like to understand why it is there for you don't mind.)

I am personally biased towards adding it to the package because it is
tree-shaking friendly, and the code shouldn't change much, and it was quite
handy for those few cases that people want to reach for sending the string from
SSR.

It could be definitely in another package, the complexity is to know that you
need to pass react/jsx-runtime and do the trick with new Function, once
you know how to do it, it is simple, but probably a lot of time researching.

I can take the lead on publishing a package, but my personal bias is more about
helping the future person that didn't know how to solve the problem or that
such an external package exists.

No preferences in the grand scheme of things


Full credit to: https://github.com/kentcdodds/mdx-bundler and @kentcdodds on this one.

@vercel
Copy link

vercel bot commented Nov 3, 2021

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

🔍 Inspect: https://vercel.com/mdx/mdx/2MvNwNBQiUTQrJERbDvg8AGpCDBh
✅ Preview: https://mdx-git-fork-yordis-yordis-add-get-info-from-functio-0ac9a6-mdx.vercel.app

@vercel vercel bot temporarily deployed to Preview November 3, 2021 08:19 Inactive
@yordis
Copy link
Contributor Author

yordis commented Nov 3, 2021

Sorry, was too late so I had to stop, I will continue the work today

@vercel vercel bot temporarily deployed to Preview November 4, 2021 11:53 Inactive
@yordis
Copy link
Contributor Author

yordis commented Nov 4, 2021

MADE IT TO WORK 🚀 🥳 🎂 🎈

P.S: Never done Preact, but will try to make it work there as well

@vercel vercel bot temporarily deployed to Preview November 4, 2021 11:55 Inactive
@vercel vercel bot temporarily deployed to Preview November 4, 2021 11:56 Inactive
@vercel vercel bot temporarily deployed to Preview November 4, 2021 11:59 Inactive
@yordis yordis marked this pull request as ready for review November 4, 2021 11:59
@yordis
Copy link
Contributor Author

yordis commented Nov 4, 2021

Should I create a separate PR for preact? probably so

@yordis
Copy link
Contributor Author

yordis commented Nov 4, 2021

CI is failing but I haven't touch "@mdx-js/node-loader" so I am not sure what is going on

@wooorm
Copy link
Member

wooorm commented Nov 4, 2021

The problem with CI is indeed unrelated to your work, it instead has to do with Node 17 breaking an experimental feature!


Some questions:

  • Can you expand on the problem you’re having, (how) this solves that, and whether there are potential alternatives?
  • Should this feature be here? In this package? Could it be somewhere else? Where?

@yordis
Copy link
Contributor Author

yordis commented Nov 5, 2021

@wooorm yeah for sure, let me get back to you on that one btw. It is pending from my side, trying my best to find some time to do things little by little.

@yordis
Copy link
Contributor Author

yordis commented Nov 7, 2021

@wooorm I updated the docs, please help me to clarify the situation if you don't understand it, I am not attached to anything, just finding the way I could build something docusaurus-like at the end.

@wooorm
Copy link
Member

wooorm commented Nov 9, 2021

Hi again Yordis, thanks for answering my questions!

So, I can understand the need to compile on the server and eval code on the client. Is it often the right way to do it? I have my doubts. eval is always dangerous. Even if it’s safe, there are performance implications. So I’m not sure if this is the right way to solve it for Next, but whether it is, is a Next problem.

Since everything is related to MDX and React, it feels the right place. [...]

The problem I have with placing it here is that we now need this same function for every framework. React, Preact, Emotion, Theme UI, etc.

This particular solution is also limited: it’s sync only. The proper way to do it would in my opinion be, similar to compile/compileSync and evaluate/evaluateSync, to have an async and sync version. I think a proper solution would be two functions. (Otherwise imports aren’t supported)
Some of the code for your solution is already available in @mdx-js/mdx, namely run and runSync: https://github.com/mdx-js/mdx/blob/main/packages/mdx/lib/run.js.

And it feels that mdx@v2 was definitely taking into consideration this since at least I found function-body in it that feels it could be used for use cases
(I would like to understand why it is there for you don't mind.)

Yep, it’s intended! function-body is particularly for evaluate, which does both compile and run. But it’s exposed so users can do what you‘re doing, which is documented at the option: https://v2.mdxjs.com/packages/mdx/#optionsoutputformat.


What if instead:

  • run and runSync are exported from @mdx-js/mdx and documented in its readme
  • we’d add a guide on how to compile on “the server”, and run on “the client” instead? https://v2.mdxjs.com/guides/

@yordis
Copy link
Contributor Author

yordis commented Nov 9, 2021

This particular solution is also limited: it’s sync only.

What would be the value of making that async? Are we talking about the evaluation or execution of the function?

I am not sure what would be the vale, would like to understand the intention.

eval is always dangerous.

Yeah ... don't disagree on that one, I am not what else to do when most people do such implementation mdx-remote does a similar thing as well 🤷🏻 I am not sure how to deal with the situation when the backend sends an string of mdx content thou.

run and runSync are exported from @mdx-js/mdx and documented in its readme

I didn't know that it exists, as long as people understand what options, I like the idea 🤔 ... how it would look like for React? just the same object I construct today?

we’d add a guide on how to compile on “the server”, and run on “the client” instead? https://v2.mdxjs.com/guides/

Love it

@wooorm
Copy link
Member

wooorm commented Nov 10, 2021

What would be the value of making that async?

Several things about MDX are async. In some cases things can be sync, but I think it’s best to default to async so that it’ll always work.
Some options add awaits to the document / function body. Notably, options.useDynamicImport can be used with options.outputFormat set to 'function-body'. That turns:

import Something from 'https://example.com/things.js'

into:

const {default: Something} = await import('https://example.com/things.js')

how it would look like for React? just the same object I construct today?

On the server:

import {compile} from '@mdx-js/mdx'

const code = await compile(file, {outputFormat: 'function-body', ...otherCompileOptions})

On the client:

import {run} from '@mdx-js/mdx'
import * as runtime from 'react/jsx-runtime'

const code = '' // get the code somewhow from the server

const {default: Component} = await run(code, runtime)

@yordis
Copy link
Contributor Author

yordis commented Nov 10, 2021

Say no more! Thank you so much for explaining, it will pay forward, TIL that I had to break an old thinking pattern

@yordis
Copy link
Contributor Author

yordis commented Nov 10, 2021

import {run} from '@mdx-js/mdx'
import * as runtime from 'react/jsx-runtime'

const code = '' // get the code somewhow from the server

const {default: Component} = await run(code, runtime)

How do you pass more global things to the context other than runtime?

Last-mile, I am leaning towards documenting things at this point

@wooorm
Copy link
Member

wooorm commented Nov 10, 2021

You can‘t! I understand you want to, but there are some downsides to globals. You can pass props instead to Content: https://mdxjs.com/docs/using-mdx/#props

@yordis
Copy link
Contributor Author

yordis commented Nov 10, 2021

That works! Given the context, I don't see an issue, and I actually LOVE that I cant

@yordis
Copy link
Contributor Author

yordis commented Nov 10, 2021

So, documentation and exposing run? I am leaning on it thou, and probably tackle (duplicate the info or link) the questions I had on it

@vercel vercel bot temporarily deployed to Preview November 12, 2021 03:42 Inactive
@yordis
Copy link
Contributor Author

yordis commented Nov 12, 2021

Before I removed the react to stuff that I added, I replaced the implementation with runSync and it worked (just personally confirming)

@wooorm I would love to get your guidelines in terms of documentation. I feel I am not as good you are, so I feel a bit impostor on this one. Gonna try to do my best.

@yordis
Copy link
Contributor Author

yordis commented Nov 12, 2021

Do I need to maintain the packages/mdx/readme.md by hand or do you use some tooling to insert the ToC and links and stuff like that?

@wooorm
Copy link
Member

wooorm commented Nov 12, 2021

Yep, packages/mdx/readme.md is written by hand. Though, the Table of Contents and other things are generated (you can run npm run format and such locally).

Here’s how I applied your idea to xdm: wooorm/xdm@710ef5f, I think you’d be able to reuse most of that prose.

Then I’ll work on the guide in a separate PR and ping you?

@yordis
Copy link
Contributor Author

yordis commented Nov 12, 2021

Then I’ll work on the guide in a separate PR and ping you?

Please do

Here’s how I applied your idea to xdm: wooorm/xdm@710ef5f, I think you’d be able to reuse most of that prose.

In bed I thought about moving the content the way you did 😆 , well, I am happy to see this becoming real.

Thank you so much!

@wooorm wooorm changed the title feat: add get mdx component to react pkg Add run, runSync exports Nov 13, 2021
@wooorm wooorm merged commit cd95df6 into mdx-js:main Nov 13, 2021
wooorm added a commit that referenced this pull request Nov 13, 2021
@yordis yordis deleted the yordis/add-get-info-from-function-body-values branch November 14, 2021 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants