react-markdown icon indicating copy to clipboard operation
react-markdown copied to clipboard

Add async processing support to support async plugins

Open mikesir87 opened this issue 3 years ago • 19 comments

Resolves #680

Initial checklist

  • [x] I read the support docs
  • [x] I read the contributing guide
  • [x] I agree to follow the code of conduct
  • [x] I searched issues and couldn’t find anything (or linked relevant results below)
  • [x] If applicable, I’ve added docs and tests

Description of changes

Added an async option to the component. If true, runs the renderer using processor.run instead of processor.runSync (which causes an error be thrown).

mikesir87 avatar Apr 06 '22 01:04 mikesir87

Codecov Report

Merging #682 (4e7fb52) into main (8143c12) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #682   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          731       745   +14     
=========================================
+ Hits           731       745   +14     
Impacted Files Coverage Δ
lib/react-markdown.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8143c12...4e7fb52. Read the comment docs.

codecov-commenter avatar Apr 06 '22 01:04 codecov-commenter

Thanks for the PR! Hmm, this seems to be a bit of a naïve attempt:

  • there are no docs
  • there is no error handling
  • there are no tests that actually test async (or event sync) plugins
  • I think this is more something for useCallback?
  • as far as I’m aware this strategy would not work when server side rendering, could there be a way around that?

wooorm avatar Apr 06 '22 09:04 wooorm

this seems to be a bit of a naïve attempt

Haha... very well might be! 😅 I tried quite a few ways to get something working that will be both backwards compatible and work for the async route.

there are no docs

My bad. I'll get on that. 😄 I did at least update the typings for the new attribute.

there is no error handling

There wasn't any previous error handling in place if the processing failed for whatever reason. Sure, in this case, it'll be an uncaught exception thrown by a Promise. Recommendations on what you'd like to see here?

there are no tests that actually test async (or event sync) plugins

I can plug in a few tests to exercise this. I did add a test that at least exercises the async rendering (why I had to add react-test-renderer).

I think this is more something for useCallback?

I'm not sure that's the case, as there's no event or callback mechanism to trigger the callback function. I actually started by running everything within a useEffect, but then that turned everything to run asynchronously (all of the unit tests then failed). So, tried to figure out a way that would work for both use cases and not violate React best practices (having conditional useEffect usage) and landed with this scenario.

as far as I’m aware this strategy would not work when server side rendering, could there be a way around that?

Based on my limited experience, it does appear that server-side rendering is unsupported for useEffect. But, it's not 100% clear if it'll work in this scenario where I'm only using useState. I don't have a SSR setup to test this out though.

My guessing is that, based on me having to use the react-test-renderer, a simple ReactDom.renderToStaticMarkup would not render the expected value.

mikesir87 avatar Apr 06 '22 10:04 mikesir87

Docs updated and added a simple async plugin to validate it both executes and the overall rendering still works.

mikesir87 avatar Apr 06 '22 10:04 mikesir87

Is this PR still alive? I'm also looking for async plugin support and wondering if this effort was ongoing. Happy to help if needed.

adamdotdevin avatar May 21 '22 12:05 adamdotdevin

There are still some more todos from https://github.com/remarkjs/react-markdown/pull/682#issuecomment-1090068072 as I understand it.

wooorm avatar May 21 '22 12:05 wooorm

As far as I'm aware (it's been a little while though), I addressed all of the concerns in the comment.

However, I did find one additional bug... if the markdown changes (eg, you have a live editor or a component is reused with new markdown), the new markdown isn't rendered. That's the challenge of trying to use the same component for both async and synchronous work. One idea is to make a completely separate component that renders asynchronously, allowing me to use useState and other hooks properly (I can't use those now because you can't have conditional hooks and using them would make all rendering async).

mikesir87 avatar May 21 '22 14:05 mikesir87

This is especially needed with the new react server components

Since the component is built on server side, the component itself can be async (as highlighted here) thus allowing function ReactMarkdown to be async in react-markdown.js. So at least for the server side component, the PR can be simplified to use run instead of runSync on the processor.

This also means that the server-side components cannot have 'useState', which means the PR probably needs to change to support server side components. @mikesir87 @wooorm

ujjwalchadha8 avatar Dec 02 '22 02:12 ujjwalchadha8

This is especially needed

Why?

as highlighted here

What you link is Next.js-specific documentation, about their own API for pages and layouts, which indeed can be async, I think that was already possible, and I don’t see how that relates to React?

wooorm avatar Dec 02 '22 06:12 wooorm

hey folks, what is the status of this PR will this be merged any time soon?

reypm avatar Jul 19 '23 00:07 reypm

@reypm you can find the status by reading the comments directly above yours. This is blocked by unresolved requests https://github.com/remarkjs/react-markdown/pull/682#issuecomment-1133627232 and by unresolved discussion https://github.com/remarkjs/react-markdown/pull/682#issuecomment-1334815897. It will be merged when and if it is ready.

ChristianMurphy avatar Jul 19 '23 01:07 ChristianMurphy

Hey @wooorm @ChristianMurphy! How can I help land this PR?

simonpfish avatar Oct 17 '23 17:10 simonpfish

We want to use a couple of async plugins in cookbook.openai.com, and this is currently blocking us. Happy to help iterate on it to be able to merge.

simonpfish avatar Oct 17 '23 17:10 simonpfish

See the comment before yours. There’s no way to do this yet, while the React RFCs aren‘t ready.


@remcohaszing had an idea that could perhaps work but would need a ton of testing and probably still be a separate “experimental“ export. Use the react-server condition, which should mean that Promise<JSX.Element> is supported. Otherwise, use a hook. That might work.

wooorm avatar Oct 17 '23 17:10 wooorm

Got it! I ended up just using unified directly and rehype-react

simonpfish avatar Oct 17 '23 19:10 simonpfish