From 6773b90d8ced05fae4ac9024e84cc31f1da9b9a3 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 27 Apr 2023 10:46:05 -0600 Subject: [PATCH] Refactor MarkdownViewer to make it SSR-friendly (#3207) * Refactor MarkdownViewer to make it SSR-friendly * Add comment about container state * Revert changes to useListInteraction * Create chilly-dragons-crash.md * Handle dom updates in useListInteraction * Handle on change error * Use isomorphic layout effect --- .changeset/chilly-dragons-crash.md | 5 +++ .../MarkdownViewer/MarkdownViewer.test.tsx | 40 +++++++++++++++++- src/drafts/MarkdownViewer/MarkdownViewer.tsx | 41 ++++++++----------- .../MarkdownViewer/_useLinkInterception.ts | 4 +- .../MarkdownViewer/_useListInteraction.ts | 28 +++++++++---- 5 files changed, 81 insertions(+), 37 deletions(-) create mode 100644 .changeset/chilly-dragons-crash.md diff --git a/.changeset/chilly-dragons-crash.md b/.changeset/chilly-dragons-crash.md new file mode 100644 index 00000000000..4dbbeace691 --- /dev/null +++ b/.changeset/chilly-dragons-crash.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +`MarkdownViewer` is now SSR-compatible diff --git a/src/drafts/MarkdownViewer/MarkdownViewer.test.tsx b/src/drafts/MarkdownViewer/MarkdownViewer.test.tsx index af975617fdc..14d4e393a79 100644 --- a/src/drafts/MarkdownViewer/MarkdownViewer.test.tsx +++ b/src/drafts/MarkdownViewer/MarkdownViewer.test.tsx @@ -1,6 +1,6 @@ -import React from 'react' -import {render, waitFor, fireEvent} from '@testing-library/react' +import {fireEvent, render, waitFor} from '@testing-library/react' import userEvent from '@testing-library/user-event' +import React from 'react' import MarkdownViewer from '../MarkdownViewer' @@ -89,6 +89,42 @@ text after list` fireEvent.change(items[0]) await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(noItemsCheckedMarkdown)) }) + + it('attaches event handlers to new tasks', async () => { + const onChangeMock = jest.fn() + const TestComponent = () => { + const [html, setHtml] = React.useState(taskListHtml) + const [markdown, setMarkdown] = React.useState(noItemsCheckedMarkdown) + return ( + <> + + + + ) + } + const {getByRole, getAllByRole} = render() + + // Change markdown and html content + const button = getByRole('button', {name: 'Add markdown'}) + fireEvent.click(button) + + const items = getAllByRole('checkbox') + fireEvent.change(items[2]) + await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(`${noItemsCheckedMarkdown}\n- [x] item 3\n`)) + }) }) describe('link interception', () => { diff --git a/src/drafts/MarkdownViewer/MarkdownViewer.tsx b/src/drafts/MarkdownViewer/MarkdownViewer.tsx index fe1636a2ae0..f34f08e25dc 100644 --- a/src/drafts/MarkdownViewer/MarkdownViewer.tsx +++ b/src/drafts/MarkdownViewer/MarkdownViewer.tsx @@ -1,5 +1,5 @@ -import React, {DOMAttributes, useCallback, useEffect, useRef, useState} from 'react' -import {Spinner, Box} from '../..' +import React, {DOMAttributes, useCallback} from 'react' +import {Box, Spinner} from '../..' import {useLinkInterception} from './_useLinkInterception' import {useListInteraction} from './_useListInteraction' @@ -55,12 +55,6 @@ type NoninteractiveMarkdownViewerProps = CoreMarkdownViewerProps & { export type MarkdownViewerProps = NoninteractiveMarkdownViewerProps | InteractiveMarkdownViewerProps -const createRenderedContainer = (html: string): HTMLDivElement => { - const div = document.createElement('div') - div.innerHTML = html - return div -} - const MarkdownViewer = ({ dangerousRenderedHTML, loading = false, @@ -70,25 +64,25 @@ const MarkdownViewer = ({ onLinkClick, openLinksInNewTab = false, }: MarkdownViewerProps) => { - const outputContainerRef = useRef(null) - - // Render the HTML into an internal container element so we can modify it before it becomes visible. - // Using `unsafeInnerHTML` would require an effect to run after rendering which would cause flicker - const [htmlContainer, setHtmlContainer] = useState(() => createRenderedContainer(dangerousRenderedHTML.__html)) - useEffect( - () => setHtmlContainer(createRenderedContainer(dangerousRenderedHTML.__html)), - [dangerousRenderedHTML.__html], - ) + // We're using state to store the HTML container because we want the value + // to re-run effects when it changes + const [htmlContainer, setHtmlContainer] = React.useState() + const htmlContainerRef = React.useCallback((node: HTMLElement | null) => { + if (!node) return + setHtmlContainer(node) + }, []) const onChange = useCallback( async (value: string) => { try { await externalOnChange?.(value) } catch (error) { - setHtmlContainer(createRenderedContainer(dangerousRenderedHTML.__html)) + if (htmlContainer) { + htmlContainer.innerHTML = dangerousRenderedHTML.__html + } } }, - [externalOnChange, dangerousRenderedHTML.__html], + [externalOnChange, htmlContainer, dangerousRenderedHTML], ) useListInteraction({ @@ -96,6 +90,7 @@ const MarkdownViewer = ({ disabled: disabled || !externalOnChange, htmlContainer, markdownValue, + dependencies: [dangerousRenderedHTML], }) useLinkInterception({ @@ -104,20 +99,16 @@ const MarkdownViewer = ({ openLinksInNewTab, }) - // If we were to inject the `...htmlContainer.children` instead of the container element itself, - // those children elements would be moved from the `htmlContainer` to the `outputContainer`. Then if - // other effects use `htmlContainer.querySelectorAll`, they wouldn't find any elements to affect - useEffect(() => outputContainerRef.current?.replaceChildren(htmlContainer), [htmlContainer]) - return loading ? ( ) : ( div > :last-child': {mb: 0}}} + dangerouslySetInnerHTML={dangerousRenderedHTML} /> ) } diff --git a/src/drafts/MarkdownViewer/_useLinkInterception.ts b/src/drafts/MarkdownViewer/_useLinkInterception.ts index 0c2e8a5f205..b5ccdccbebb 100644 --- a/src/drafts/MarkdownViewer/_useLinkInterception.ts +++ b/src/drafts/MarkdownViewer/_useLinkInterception.ts @@ -1,7 +1,7 @@ import {useEffect} from 'react' type UseLinkInterceptionSettings = { - htmlContainer: HTMLDivElement + htmlContainer?: HTMLElement onLinkClick?: (event: MouseEvent) => void openLinksInNewTab: boolean } @@ -23,6 +23,8 @@ export const useLinkInterception = ({htmlContainer, onLinkClick, openLinksInNewT } } + if (!htmlContainer) return + htmlContainer.addEventListener('click', clickHandler) return () => { diff --git a/src/drafts/MarkdownViewer/_useListInteraction.ts b/src/drafts/MarkdownViewer/_useListInteraction.ts index 1f7c461ed19..1ce972970a1 100644 --- a/src/drafts/MarkdownViewer/_useListInteraction.ts +++ b/src/drafts/MarkdownViewer/_useListInteraction.ts @@ -1,4 +1,5 @@ -import {useCallback, useEffect, useLayoutEffect, useMemo, useRef} from 'react' +import {useCallback, useEffect, useRef, useState} from 'react' +import useIsomorphicLayoutEffect from '../../utils/useIsomorphicLayoutEffect' import {ListItem, listItemToString, parseListItem} from '../MarkdownEditor/_useListEditing' type TaskListItem = ListItem & {taskBox: '[ ]' | '[x]'} @@ -11,10 +12,11 @@ const toggleTaskListItem = (item: TaskListItem): TaskListItem => ({ }) type UseListInteractionSettings = { - htmlContainer: HTMLDivElement | null + htmlContainer?: HTMLElement markdownValue: string onChange: (markdown: string) => void | Promise disabled?: boolean + dependencies?: Array } /** @@ -28,11 +30,13 @@ export const useListInteraction = ({ markdownValue, onChange, disabled = false, + dependencies = [], }: UseListInteractionSettings) => { // Storing the value in a ref allows not using the markdown value as a depdency of // onToggleItem, which would mean we'd have to re-bind the event handlers on every change const markdownRef = useRef(markdownValue) - useLayoutEffect(() => { + + useIsomorphicLayoutEffect(() => { markdownRef.current = markdownValue }, [markdownValue]) @@ -62,12 +66,18 @@ export const useListInteraction = ({ [onChange], ) - const checkboxElements = useMemo( - () => - Array.from( - htmlContainer?.querySelectorAll('input[type=checkbox].task-list-item-checkbox') ?? [], - ), - [htmlContainer], + const [checkboxElements, setCheckboxElements] = useState([]) + + useEffect( + () => { + setCheckboxElements( + Array.from( + htmlContainer?.querySelectorAll('input[type=checkbox].task-list-item-checkbox') ?? [], + ), + ) + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [htmlContainer, ...dependencies], ) // This could be combined with the other effect, but then the checkboxes might have a flicker