Add support for async plugins #890
Conversation
This commit adds 2 new components that support turning markdown into react nodes, asynchronously. There are different ways to support async things in React. Component with hooks only run on the client. Components yielding promises are not supported on the client. To support different scenarios and the different ways the future could develop, these choices are made explicit to users. Users can choose whether `MarkdownAsync` or `MarkdownHooks` fits their use case. Closes GH-680. Closes GH-682.
github-actions
bot
added
👋 phase/new
|
worth noting that I tested this on the demo/playground here and it works well |
readme.md
Outdated
| ### `MarkdownHooks` | ||
|
|
||
| Component to render markdown with support for async plugins through hooks. | ||
|
|
||
| Hooks run on the client. | ||
| For async support on the server, | ||
| see [`MarkdownAsync`][api-markdown-async]. |
There was a problem hiding this comment.
Could we expand on this a bit? Including that this uses useState, and so will not immediately render on initial page load?
| * React element. | ||
| */ | ||
| export function MarkdownHooks(options) { | ||
| const processor = createProcessor(options) |
There was a problem hiding this comment.
The processor is only needed inside the useEffect. We can move it there as well. That means the processor isn’t created on every render and doesn’t need to be in the useEffect dependency array.
There was a problem hiding this comment.
Isn’t Object.is used for the dependencies? And because Object.is({}, {}) // false, that would mean putting a created processor in dependencies would never be equal?
| const file = createFile(options) | ||
| setPromise(processor.run(processor.parse(file), file)) | ||
| }, | ||
| [options.children] |
There was a problem hiding this comment.
The dependency should specify all component local values needed to run the effect.
There was a problem hiding this comment.
you want an array of every field in options?
There was a problem hiding this comment.
that’s why processor is outside.
There was a problem hiding this comment.
I don’t know what you mean by “component local values” — can you use (pseudo)code to explain your ideas?
There was a problem hiding this comment.
Currently processor needs to be in the dependency array, because it’s defined inside MarkdownHooks. That will trigger infinite rerenders.
The real fix is to move the creation of processor inside the useEffect. and add all values needed to create it to the dependency array.
Best would be to enable eslint-plugin-react-hooks.
There was a problem hiding this comment.
So you are suggesting to add every field on options into the dependency array?
The caveats describe to not put objects in dependencies: https://react.dev/reference/react/useEffect#caveats.
| const tree = use(promise) | ||
|
|
||
| /* c8 ignore next -- hooks are client-only. */ | ||
| return tree ? post(tree, options) : createElement(Fragment) |
There was a problem hiding this comment.
You don’t have to return an empty fragment here. It’s sufficient to return undefined.
There was a problem hiding this comment.
That would make the types for only this function different from the other ones
Initial checklist
Description of changes
This commit adds 2 new components that support
turning markdown into react nodes,
asynchronously.
There are different ways to support async things in React. Component with hooks only run on the client.
Components yielding promises are not supported on the client. To support different scenarios and the different ways the future could develop,
these choices are made explicit to users.
Users can choose whether
MarkdownAsyncorMarkdownHooksfits their use case.Closes GH-680.
Closes GH-682.