From 468ca0f9b3388878a30f05996d81af0351f943ce Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:03:38 +0200 Subject: [PATCH 1/7] fix(techdocs): fixed race condition between page loads --- .../techdocs/src/reader/components/Reader.tsx | 21 ++++++++----- .../techdocs/src/reader/hooks/shadowDom.ts | 30 ++++++++++++++----- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index 995d9dc99f..bd7ac17a6a 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -35,15 +35,22 @@ import { TechDocsPageWrapper } from './TechDocsPageWrapper'; const useFetch = (url: string): AsyncState => { const state = useAsync(async () => { - const response = await fetch(url); - if (response.status === 404) { - return new Error('Page not found'); + const request = await fetch(url); + if (request.status === 404) { + return [request.url, new Error('Page not found')]; } - const raw = await response.text(); - return raw; + const response = await request.text(); + return [request.url, response]; }, [url]); - return state; + const [fetchedUrl, fetchedValue] = state.value ?? []; + + if (url !== fetchedUrl) { + // Fixes a race condition between two pages + return { loading: true }; + } + + return Object.assign(state, fetchedValue ? { value: fetchedValue } : {}); }; const useEnforcedTrailingSlash = (): void => { @@ -60,7 +67,7 @@ const useEnforcedTrailingSlash = (): void => { export const Reader = () => { const location = useLocation(); const { componentId, '*': path } = useParams(); - const shadowDomRef = useShadowDom(); + const [ref, shadowRoot] = useShadowDom(componentId, path); const navigate = useNavigate(); const normalizedUrl = new URLFormatter( `${docStorageURL}${location.pathname.replace('/docs', '')}`, diff --git a/plugins/techdocs/src/reader/hooks/shadowDom.ts b/plugins/techdocs/src/reader/hooks/shadowDom.ts index 5edb8f4e4d..90b287e343 100644 --- a/plugins/techdocs/src/reader/hooks/shadowDom.ts +++ b/plugins/techdocs/src/reader/hooks/shadowDom.ts @@ -17,14 +17,30 @@ import { useEffect, useRef } from 'react'; import type { RefObject } from 'react'; -type IShadowDOMRefObject = RefObject; -export const useShadowDom: () => IShadowDOMRefObject = () => { - const ref: IShadowDOMRefObject = useRef(null); +type IUseShadowDOM = ( + componentId: string, + path: string, +) => [RefObject, ShadowRoot?]; + +export const useShadowDom: IUseShadowDOM = (componentId, path) => { + const ref = useRef(null); useEffect(() => { - const divElement = ref.current; - divElement?.attachShadow({ mode: 'open' }); - }, [ref]); + const innerDivElement = document.createElement('div'); + innerDivElement.attachShadow({ mode: 'open' }); - return ref; + const outerDivElement = ref.current; + outerDivElement?.appendChild(innerDivElement); + + return function cancel() { + outerDivElement?.removeChild(innerDivElement); + }; + }, [componentId, path]); + + const shadowRoot = + ref.current?.children && ref.current?.children[0].shadowRoot + ? ref.current?.children[0].shadowRoot + : undefined; + + return [ref, shadowRoot]; }; From 96c24c687b4e3e32461a97531aac9acc68dd91fb Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:04:00 +0200 Subject: [PATCH 2/7] chore(techdocs): updated tests --- .../src/reader/hooks/shadowDom.test.tsx | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx b/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx index 1d58bf26e7..b8a6a78c1f 100644 --- a/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx +++ b/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx @@ -19,26 +19,31 @@ import { renderWithEffects } from '@backstage/test-utils'; import { useShadowDom } from './shadowDom'; const ComponentWithoutHook = () => { - return
; + return
; }; const ComponentWithHook = () => { - const ref = useShadowDom(); - return
; + const [ref] = useShadowDom('mkdocs', 'about/license/'); + return
; }; describe('useShadowDom', () => { it('does not create a Shadow DOM instance', async () => { const rendered = await renderWithEffects(); - const divElement = rendered.getByTestId('shadow-dom'); - expect(divElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); + const outerDivElement = rendered.getByTestId('root'); + expect(outerDivElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); + expect(outerDivElement.children.length).toBe(0); }); it('create a Shadow DOM instance', async () => { const rendered = await renderWithEffects(); - const divElement = rendered.getByTestId('shadow-dom'); - expect(divElement.shadowRoot).toBeInstanceOf(ShadowRoot); + const outerDivElement = rendered.getByTestId('root'); + expect(outerDivElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); + expect(outerDivElement.children.length).toBe(1); + + const innerDivElement = outerDivElement.children[0]; + expect(innerDivElement.shadowRoot).toBeInstanceOf(ShadowRoot); }); }); From 95ebc8eba9e44ba2c19a56552bee2b4db054c6a2 Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:04:26 +0200 Subject: [PATCH 3/7] fix: moved useEnforcingTrailingSlash upper in the hook chain --- plugins/techdocs/src/reader/components/Reader.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index bd7ac17a6a..d3aadae6aa 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -65,6 +65,8 @@ const useEnforcedTrailingSlash = (): void => { }; export const Reader = () => { + useEnforcedTrailingSlash(); + const location = useLocation(); const { componentId, '*': path } = useParams(); const [ref, shadowRoot] = useShadowDom(componentId, path); @@ -74,8 +76,6 @@ export const Reader = () => { ).formatBaseURL(); const state = useFetch(`${normalizedUrl}index.html`); - useEnforcedTrailingSlash(); - React.useEffect(() => { const divElement = shadowDomRef.current; if ( From 52f4d37b8ea9ad212dba5fb0dc640131547eeab9 Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:05:12 +0200 Subject: [PATCH 4/7] fix(techdocs): cleaned up useEffect in Reader --- .../techdocs/src/reader/components/Reader.tsx | 111 +++++++++--------- 1 file changed, 58 insertions(+), 53 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index d3aadae6aa..31ccbbb57a 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -77,66 +77,71 @@ export const Reader = () => { const state = useFetch(`${normalizedUrl}index.html`); React.useEffect(() => { - const divElement = shadowDomRef.current; - if ( - divElement?.shadowRoot && - state.value && - !(state.value instanceof Error) - ) { - const transformedElement = transformer(state.value, [ - addBaseUrl({ - docStorageURL, - componentId, - path, - }), - rewriteDocLinks(), - modifyCss({ - cssTransforms: { - '.md-main__inner': [{ 'margin-top': '0' }], - '.md-sidebar': [{ top: '0' }, { width: '20rem' }], - '.md-typeset': [{ 'font-size': '1rem' }], - '.md-nav': [{ 'font-size': '1rem' }], - '.md-grid': [{ 'max-width': '80vw' }], - }, - }), - removeMkdocsHeader(), - ]); - - divElement.shadowRoot.innerHTML = ''; - - if (transformedElement) { - divElement.shadowRoot.appendChild(transformedElement); - transformer(divElement.shadowRoot.children[0], [ - dom => { - setTimeout(() => { - if (window.location.hash) { - const hash = window.location.hash.slice(1); - divElement.shadowRoot?.getElementById(hash)?.scrollIntoView(); - } - }, 200); - return dom; - }, - addLinkClickListener({ - onClick: (_: MouseEvent, url: string) => { - const parsedUrl = new URL(url); - navigate(`${parsedUrl.pathname}${parsedUrl.hash}`); - - divElement.shadowRoot - ?.querySelector(parsedUrl.hash) - ?.scrollIntoView(); - }, - }), - ]); - } + if (!shadowRoot) { + return; // Shadow DOM isn't ready } - }, [shadowDomRef, state, componentId, path, navigate]); + + if (state.loading) { + return; // Page isn't ready + } + + // Pre-render + const transformedElement = transformer(state.value as string, [ + addBaseUrl({ + docStorageURL, + componentId, + path, + }), + rewriteDocLinks(), + modifyCss({ + cssTransforms: { + '.md-main__inner': [{ 'margin-top': '0' }], + '.md-sidebar': [{ top: '0' }, { width: '20rem' }], + '.md-typeset': [{ 'font-size': '1rem' }], + '.md-nav': [{ 'font-size': '1rem' }], + '.md-grid': [{ 'max-width': '80vw' }], + }, + }), + removeMkdocsHeader(), + ]); + + if (!transformedElement) { + return; // An unexpected error occurred + } + + Array.from(shadowRoot.children).forEach(child => + shadowRoot.removeChild(child), + ); + shadowRoot.appendChild(transformedElement); + + // Post-render + transformer(shadowRoot.children[0], [ + dom => { + setTimeout(() => { + if (window.location.hash) { + const hash = window.location.hash.slice(1); + shadowRoot?.getElementById(hash)?.scrollIntoView(); + } + }, 200); + return dom; + }, + addLinkClickListener({ + onClick: (_: MouseEvent, url: string) => { + const parsedUrl = new URL(url); + navigate(`${parsedUrl.pathname}${parsedUrl.hash}`); + + shadowRoot?.querySelector(parsedUrl.hash)?.scrollIntoView(); + }, + }), + ]); + }, [componentId, path, shadowRoot, state]); // eslint-disable-line react-hooks/exhaustive-deps if (state.value instanceof Error) return ; return ( <> -
+
); From a662bc1639cfb91c19849481d0c37b6bd11b14ad Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:18:34 +0200 Subject: [PATCH 5/7] fix: remove re-creating new shadow-dom on page change --- .../techdocs/src/reader/components/Reader.tsx | 2 +- .../src/reader/hooks/shadowDom.test.tsx | 15 ++++------- .../techdocs/src/reader/hooks/shadowDom.ts | 27 +++++-------------- 3 files changed, 12 insertions(+), 32 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index 31ccbbb57a..4f89b9296f 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -69,7 +69,7 @@ export const Reader = () => { const location = useLocation(); const { componentId, '*': path } = useParams(); - const [ref, shadowRoot] = useShadowDom(componentId, path); + const [ref, shadowRoot] = useShadowDom(); const navigate = useNavigate(); const normalizedUrl = new URLFormatter( `${docStorageURL}${location.pathname.replace('/docs', '')}`, diff --git a/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx b/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx index b8a6a78c1f..31956ccb06 100644 --- a/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx +++ b/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx @@ -23,7 +23,7 @@ const ComponentWithoutHook = () => { }; const ComponentWithHook = () => { - const [ref] = useShadowDom('mkdocs', 'about/license/'); + const [ref] = useShadowDom(); return
; }; @@ -31,19 +31,14 @@ describe('useShadowDom', () => { it('does not create a Shadow DOM instance', async () => { const rendered = await renderWithEffects(); - const outerDivElement = rendered.getByTestId('root'); - expect(outerDivElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); - expect(outerDivElement.children.length).toBe(0); + const divElement = rendered.getByTestId('root'); + expect(divElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); }); it('create a Shadow DOM instance', async () => { const rendered = await renderWithEffects(); - const outerDivElement = rendered.getByTestId('root'); - expect(outerDivElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); - expect(outerDivElement.children.length).toBe(1); - - const innerDivElement = outerDivElement.children[0]; - expect(innerDivElement.shadowRoot).toBeInstanceOf(ShadowRoot); + const divElement = rendered.getByTestId('root'); + expect(divElement.shadowRoot).toBeInstanceOf(ShadowRoot); }); }); diff --git a/plugins/techdocs/src/reader/hooks/shadowDom.ts b/plugins/techdocs/src/reader/hooks/shadowDom.ts index 90b287e343..93e4c1d375 100644 --- a/plugins/techdocs/src/reader/hooks/shadowDom.ts +++ b/plugins/techdocs/src/reader/hooks/shadowDom.ts @@ -17,30 +17,15 @@ import { useEffect, useRef } from 'react'; import type { RefObject } from 'react'; -type IUseShadowDOM = ( - componentId: string, - path: string, -) => [RefObject, ShadowRoot?]; +type IUseShadowDOM = () => [RefObject, ShadowRoot?]; -export const useShadowDom: IUseShadowDOM = (componentId, path) => { +export const useShadowDom: IUseShadowDOM = () => { const ref = useRef(null); useEffect(() => { - const innerDivElement = document.createElement('div'); - innerDivElement.attachShadow({ mode: 'open' }); + const divElement = ref.current; + divElement?.attachShadow({ mode: 'open' }); + }, []); - const outerDivElement = ref.current; - outerDivElement?.appendChild(innerDivElement); - - return function cancel() { - outerDivElement?.removeChild(innerDivElement); - }; - }, [componentId, path]); - - const shadowRoot = - ref.current?.children && ref.current?.children[0].shadowRoot - ? ref.current?.children[0].shadowRoot - : undefined; - - return [ref, shadowRoot]; + return [ref, ref.current?.shadowRoot || undefined]; }; From 07e5d2ce8a3943f8884989d721c6aee5879a2902 Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:25:09 +0200 Subject: [PATCH 6/7] chore: revert data-testid back to shadow-dom --- plugins/techdocs/src/reader/hooks/shadowDom.test.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx b/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx index 31956ccb06..c4fab0dbc6 100644 --- a/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx +++ b/plugins/techdocs/src/reader/hooks/shadowDom.test.tsx @@ -19,26 +19,26 @@ import { renderWithEffects } from '@backstage/test-utils'; import { useShadowDom } from './shadowDom'; const ComponentWithoutHook = () => { - return
; + return
; }; const ComponentWithHook = () => { const [ref] = useShadowDom(); - return
; + return
; }; describe('useShadowDom', () => { it('does not create a Shadow DOM instance', async () => { const rendered = await renderWithEffects(); - const divElement = rendered.getByTestId('root'); + const divElement = rendered.getByTestId('shadow-dom'); expect(divElement.shadowRoot).not.toBeInstanceOf(ShadowRoot); }); it('create a Shadow DOM instance', async () => { const rendered = await renderWithEffects(); - const divElement = rendered.getByTestId('root'); + const divElement = rendered.getByTestId('shadow-dom'); expect(divElement.shadowRoot).toBeInstanceOf(ShadowRoot); }); }); From 9e4c092d6b0ed6735c4371b34a8303ef6c7274df Mon Sep 17 00:00:00 2001 From: Bilawal Hameed Date: Fri, 3 Jul 2020 16:26:54 +0200 Subject: [PATCH 7/7] chore: ref to shadowDomRef --- plugins/techdocs/src/reader/components/Reader.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index 4f89b9296f..42be2732d0 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -69,7 +69,7 @@ export const Reader = () => { const location = useLocation(); const { componentId, '*': path } = useParams(); - const [ref, shadowRoot] = useShadowDom(); + const [shadowDomRef, shadowRoot] = useShadowDom(); const navigate = useNavigate(); const normalizedUrl = new URLFormatter( `${docStorageURL}${location.pathname.replace('/docs', '')}`, @@ -141,7 +141,7 @@ export const Reader = () => { return ( <> -
+
);