Avoid bad hooks dependency usage in MarkdownHooks #891
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.
As some comments point out in #890, there are some bad React hook usages in those changes. A `useEffect()`, `useMemo()`, or `useCallback()` should always specify all of its dependencies in the dependency array. But the dependencies need to be referentially equal in order to not trigger the hook, which is often not possible, given that plugin arrays are often created during render. This change replaces the `useEffect()` logic with a `useRef()`. Using `useRef()`, we can persist an object instance across rerenders. We can then perform our own props comparison logic to update or not instead of relying on a dependency array. We also avoid `setState()`, which triggered another render and could cause flashes of empty content. The processor is updated if `remarkRehypeOptions`, `rehypePlugins`, or `remarkPlugins` changed. The promise is updated if the processor or `children` has changed.
github-actions
bot
added
👋 phase/new
|
I realized the |
There was a problem hiding this comment.
I think it is sad that we would implement such caching equivalence mechanisms; I believe that is what frameworks are supposed to do for you.
As you know, caching is difficult. That shows here too as there are several problems with this equivalence function: a) plugins support several parameters not just options; b) presets.
But, most importantly: you only have a return true on L464: you can remove the entire function as it will always be the tail return false, just with more or less work.
I somewhat agree here. Just React works great, but it can be hard to integrate external libraries that weren’t built with the React mindset. This would be a lot simpler if we didn’t allow
Good finds! This is really going to need tests. |
This comment has been minimized.
This comment has been minimized.
Initial checklist
Description of changes
As some comments point out in #890, there are some bad React hook usages in those changes. A
useEffect(),useMemo(), oruseCallback()should always specify all of its dependencies in the dependency array. But the dependencies need to be referentially equal in order to not trigger the hook, which is often not possible, given that plugin arrays are often created during render.This change replaces the
useEffect()logic with auseRef(). UsinguseRef(), we can persist an object instance across rerenders. We can then perform our own props comparison logic to update or not instead of relying on a dependency array. We also avoidsetState(), which triggered another render and could cause flashes of empty content.The processor is updated if
remarkRehypeOptions,rehypePlugins, orremarkPluginschanged. The promise is updated if the processor orchildrenhas changed.TODO
remarkRehypeOptions??