From 378cc6a54bdb3f4f149201c32b018d7f93f3b560 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Thu, 15 Jul 2021 11:22:31 +0200 Subject: [PATCH] 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(),