From 378cc6a54bdb3f4f149201c32b018d7f93f3b560 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Thu, 15 Jul 2021 11:22:31 +0200 Subject: [PATCH 1/3] Only update the path when the content is updated Signed-off-by: Dominik Henneke --- .changeset/techdocs-fifty-cameras-listen.md | 5 + .../techdocs/src/reader/components/Reader.tsx | 4 +- .../reader/components/useReaderState.test.tsx | 160 +++++++++++++++--- .../src/reader/components/useReaderState.ts | 26 ++- 4 files changed, 157 insertions(+), 38 deletions(-) create mode 100644 .changeset/techdocs-fifty-cameras-listen.md diff --git a/.changeset/techdocs-fifty-cameras-listen.md b/.changeset/techdocs-fifty-cameras-listen.md new file mode 100644 index 0000000000..9071004acd --- /dev/null +++ b/.changeset/techdocs-fifty-cameras-listen.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Only update the `path` when the content is updated. diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index d63ddf8125..76349784e7 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -47,17 +47,17 @@ type Props = { export const Reader = ({ entityId, onReady }: Props) => { const { kind, namespace, name } = entityId; - const { '*': path } = useParams(); const theme = useTheme(); const { state, + path, contentReload, content: rawPage, contentErrorMessage, syncErrorMessage, buildLog, - } = useReaderState(kind, namespace, name, path); + } = useReaderState(kind, namespace, name, useParams()['*']); const techdocsStorageApi = useApi(techdocsStorageApiRef); const [sidebars, setSidebars] = useState(); diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index 9d9a8e2b1a..ebc9277096 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -86,7 +86,7 @@ describe('useReaderState', () => { }; it('should return a copy of the state', () => { - expect(reducer(oldState, { type: 'navigate', path: '/' })).toEqual({ + expect(reducer(oldState, { type: 'content', path: '/' })).toEqual({ activeSyncState: 'CHECKING', contentLoading: false, path: '/', @@ -102,13 +102,11 @@ describe('useReaderState', () => { }); it.each` - type | oldActiveSyncState | newActiveSyncState - ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'navigate'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'navigate'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'sync'} | ${'BUILD_READY'} | ${undefined} - ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} + type | oldActiveSyncState | newActiveSyncState + ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'sync'} | ${'BUILD_READY'} | ${undefined} + ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} `( 'should, when type=$type and activeSyncState=$oldActiveSyncState, set activeSyncState=$newActiveSyncState', ({ type, oldActiveSyncState, newActiveSyncState }) => { @@ -164,6 +162,27 @@ describe('useReaderState', () => { }); }); + it('should set content and update path', () => { + expect( + reducer( + { + ...oldState, + contentLoading: true, + }, + { + type: 'content', + content: 'asdf', + path: '/new-path', + }, + ), + ).toEqual({ + ...oldState, + contentLoading: false, + content: 'asdf', + path: '/new-path', + }); + }); + it('should set error', () => { expect( reducer( @@ -185,20 +204,6 @@ describe('useReaderState', () => { }); }); - describe('"navigate" action', () => { - it('should work', () => { - expect( - reducer(oldState, { - type: 'navigate', - path: '/', - }), - ).toEqual({ - ...oldState, - path: '/', - }); - }); - }); - describe('"sync" action', () => { it('should update state', () => { expect( @@ -256,6 +261,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -267,6 +273,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -313,6 +320,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -324,6 +332,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'INITIAL_BUILD', + path: '/example', content: undefined, contentErrorMessage: 'NotFoundError: Page Not Found', syncErrorMessage: undefined, @@ -335,6 +344,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -346,6 +356,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -394,6 +405,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -405,6 +417,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -416,6 +429,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_REFRESHING', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -427,6 +441,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_READY', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -441,6 +456,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -452,6 +468,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my new content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -475,6 +492,103 @@ describe('useReaderState', () => { }); }); + it('should handle navigation', async () => { + techdocsStorageApi.getEntityDocs + .mockResolvedValueOnce('my content') + .mockImplementationOnce(async () => { + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'my new content'; + }) + .mockRejectedValueOnce(new NotFoundError('Some error description')); + techdocsStorageApi.syncEntityDocs.mockResolvedValue('cached'); + + await act(async () => { + const { result, waitForValueToChange, rerender } = await renderHook( + ({ path }: { path: string }) => + useReaderState('Component', 'default', 'backstage', path), + { initialProps: { path: '/example' }, wrapper: Wrapper as any }, + ); + + expect(result.current).toEqual({ + state: 'CHECKING', + path: '/example', + content: undefined, + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // show the content + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + path: '/example', + content: 'my content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // navigate + rerender({ path: '/new' }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CHECKING', + path: '/example', + content: undefined, + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + path: '/new', + content: 'my new content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // navigate + rerender({ path: '/missing' }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_NOT_FOUND', + path: '/missing', + content: undefined, + contentErrorMessage: 'NotFoundError: Some error description', + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/new', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); + }); + }); + it('should handle content error', async () => { techdocsStorageApi.getEntityDocs.mockRejectedValue( new NotFoundError('Some error description'), @@ -489,6 +603,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -500,6 +615,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_NOT_FOUND', + path: '/example', content: undefined, contentErrorMessage: 'NotFoundError: Some error description', syncErrorMessage: undefined, diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index 178c0746bf..fabcfa2687 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -15,7 +15,7 @@ */ import { useApi } from '@backstage/core-plugin-api'; -import { useEffect, useMemo, useReducer, useRef } from 'react'; +import { useMemo, useReducer, useRef } from 'react'; import { useAsync, useAsyncRetry } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; @@ -133,11 +133,11 @@ type ReducerActions = } | { type: 'content'; + path?: string; content?: string; contentLoading?: true; contentError?: Error; } - | { type: 'navigate'; path: string } | { type: 'buildLog'; log: string }; type ReducerState = { @@ -187,15 +187,15 @@ export function reducer( break; case 'content': + if (typeof action.path === 'string') { + newState.path = action.path; + } + newState.content = action.content; newState.contentLoading = action.contentLoading ?? false; newState.contentError = action.contentError; break; - case 'navigate': - newState.path = action.path; - break; - case 'buildLog': newState.buildLog = newState.buildLog.concat(action.log); break; @@ -207,7 +207,7 @@ export function reducer( // a navigation or a content update loads fresh content so the build is updated to being up-to-date if ( ['BUILD_READY', 'BUILD_READY_RELOAD'].includes(newState.activeSyncState) && - ['content', 'navigate'].includes(action.type) + ['content'].includes(action.type) ) { newState.activeSyncState = 'UP_TO_DATE'; newState.buildLog = []; @@ -223,6 +223,7 @@ export function useReaderState( path: string, ): { state: ContentStateTypes; + path: string; contentReload: () => void; content?: string; contentErrorMessage?: string; @@ -238,11 +239,6 @@ export function useReaderState( const techdocsStorageApi = useApi(techdocsStorageApiRef); - // convert all path changes into actions - useEffect(() => { - dispatch({ type: 'navigate', path }); - }, [path]); - // try to load the content. the function will fire events and we don't care for the return values const { retry: contentReload } = useAsyncRetry(async () => { dispatch({ type: 'content', contentLoading: true }); @@ -253,11 +249,12 @@ export function useReaderState( path, ); - dispatch({ type: 'content', content: entityDocs }); + // update content and path at the same time + dispatch({ type: 'content', content: entityDocs, path }); return entityDocs; } catch (e) { - dispatch({ type: 'content', contentError: e }); + dispatch({ type: 'content', contentError: e, path }); } return undefined; @@ -335,6 +332,7 @@ export function useReaderState( return { state: displayState, contentReload, + path: state.path, content: state.content, contentErrorMessage: state.contentError?.toString(), syncErrorMessage: state.syncError?.toString(), From 3d3c43f0a70337fa6db69da2387088f2acce4705 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Thu, 15 Jul 2021 14:05:51 +0200 Subject: [PATCH 2/3] Create a dedicated `contentLoading` action that keeps old content This change makes sure that the content is removed when an error occurs such as a link to a missing page. It also stills shows the old content while the new one is loaded. The (delayed) loading indicator still shows that new content is loaded. Signed-off-by: Dominik Henneke --- .../techdocs/src/reader/components/Reader.tsx | 4 ++ .../reader/components/useReaderState.test.tsx | 51 +++++++++++++++---- .../src/reader/components/useReaderState.ts | 18 +++++-- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index 76349784e7..f981394036 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -96,6 +96,10 @@ export const Reader = ({ entityId, onReady }: Props) => { useEffect(() => { if (!rawPage || !shadowDomRef.current) { + // clear the shadow dom if no content is available + if (shadowDomRef.current?.shadowRoot) { + shadowDomRef.current.shadowRoot.innerHTML = ''; + } return; } if (onReady) { diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index ebc9277096..b0d4e0326a 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -102,11 +102,13 @@ describe('useReaderState', () => { }); it.each` - type | oldActiveSyncState | newActiveSyncState - ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'sync'} | ${'BUILD_READY'} | ${undefined} - ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} + type | oldActiveSyncState | newActiveSyncState + ${'contentLoading'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'contentLoading'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'sync'} | ${'BUILD_READY'} | ${undefined /* undefined, because we don't set an input */} + ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined /* undefined, because we don't set an input */} `( 'should, when type=$type and activeSyncState=$oldActiveSyncState, set activeSyncState=$newActiveSyncState', ({ type, oldActiveSyncState, newActiveSyncState }) => { @@ -122,18 +124,45 @@ describe('useReaderState', () => { }, ); - describe('"content" action', () => { + describe('"contentLoading" action', () => { it('should set loading', () => { + expect( + reducer(oldState, { + type: 'contentLoading', + }), + ).toEqual({ + ...oldState, + contentLoading: true, + }); + }); + + it('should keep content', () => { expect( reducer( { ...oldState, content: 'some-old-content', + }, + { + type: 'contentLoading', + }, + ), + ).toEqual({ + ...oldState, + contentLoading: true, + content: 'some-old-content', + }); + }); + + it('should reset errors', () => { + expect( + reducer( + { + ...oldState, contentError: new Error(), }, { - type: 'content', - contentLoading: true, + type: 'contentLoading', }, ), ).toEqual({ @@ -141,7 +170,9 @@ describe('useReaderState', () => { contentLoading: true, }); }); + }); + describe('"content" action', () => { it('should set content', () => { expect( reducer( @@ -457,7 +488,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', path: '/example', - content: undefined, + content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, buildLog: [], @@ -538,7 +569,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', path: '/example', - content: undefined, + content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, buildLog: [], diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index fabcfa2687..07bd338de9 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -131,11 +131,11 @@ type ReducerActions = state: SyncStates; syncError?: Error; } + | { type: 'contentLoading' } | { type: 'content'; path?: string; content?: string; - contentLoading?: true; contentError?: Error; } | { type: 'buildLog'; log: string }; @@ -186,13 +186,21 @@ export function reducer( newState.syncError = action.syncError; break; + case 'contentLoading': + newState.contentLoading = true; + + // only reset errors but keep the old content until it is replaced by the 'content' action + newState.contentError = undefined; + break; + case 'content': + // only override the path if it is part of the action if (typeof action.path === 'string') { newState.path = action.path; } + newState.contentLoading = false; newState.content = action.content; - newState.contentLoading = action.contentLoading ?? false; newState.contentError = action.contentError; break; @@ -204,10 +212,10 @@ export function reducer( throw new Error(); } - // a navigation or a content update loads fresh content so the build is updated to being up-to-date + // a content update loads fresh content so the build is updated to being up-to-date if ( ['BUILD_READY', 'BUILD_READY_RELOAD'].includes(newState.activeSyncState) && - ['content'].includes(action.type) + ['contentLoading', 'content'].includes(action.type) ) { newState.activeSyncState = 'UP_TO_DATE'; newState.buildLog = []; @@ -241,7 +249,7 @@ export function useReaderState( // try to load the content. the function will fire events and we don't care for the return values const { retry: contentReload } = useAsyncRetry(async () => { - dispatch({ type: 'content', contentLoading: true }); + dispatch({ type: 'contentLoading' }); try { const entityDocs = await techdocsStorageApi.getEntityDocs( From 9b1524ad3348c3f2a69ef1ba8b08d7853710ee75 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 19 Jul 2021 16:55:44 +0200 Subject: [PATCH 3/3] Update changeset Signed-off-by: Dominik Henneke --- .changeset/techdocs-fifty-cameras-listen.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/techdocs-fifty-cameras-listen.md b/.changeset/techdocs-fifty-cameras-listen.md index 9071004acd..fb6be9d709 100644 --- a/.changeset/techdocs-fifty-cameras-listen.md +++ b/.changeset/techdocs-fifty-cameras-listen.md @@ -3,3 +3,5 @@ --- Only update the `path` when the content is updated. +If content and path are updated independently, the frontend rendering is triggered twice on each navigation: Once for the `path` change (with the old content) and once for the new content. +This might result in a flickering rendering that is caused by the async frontend preprocessing, and the fact that replacing the shadow dom content is expensive.