From ba984f675a121df2260e3a7c717d59a8448cf219 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 16 Jun 2021 11:04:47 +0200 Subject: [PATCH] Add a BUILD_READY_RELOAD type that replaces the old contentIsStale logic Signed-off-by: Dominik Henneke --- .../reader/components/useReaderState.test.tsx | 271 +++++++++--------- .../src/reader/components/useReaderState.ts | 114 ++++---- 2 files changed, 180 insertions(+), 205 deletions(-) diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index d5579ddd5a..8a09241588 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -49,20 +49,22 @@ describe('useReaderState', () => { describe('calculateDisplayState', () => { it.each` - contentLoading | content | activeSyncState | expected - ${true} | ${''} | ${''} | ${'CHECKING'} - ${false} | ${undefined} | ${'CHECKING'} | ${'CHECKING'} - ${false} | ${undefined} | ${'BUILDING'} | ${'INITIAL_BUILD'} - ${false} | ${undefined} | ${'BUILD_READY'} | ${'CONTENT_NOT_FOUND'} - ${false} | ${undefined} | ${'BUILD_TIMED_OUT'} | ${'CONTENT_NOT_FOUND'} - ${false} | ${undefined} | ${'UP_TO_DATE'} | ${'CONTENT_NOT_FOUND'} - ${false} | ${undefined} | ${'ERROR'} | ${'CONTENT_NOT_FOUND'} - ${false} | ${'asdf'} | ${'CHECKING'} | ${'CONTENT_FRESH'} - ${false} | ${'asdf'} | ${'BUILDING'} | ${'CONTENT_STALE_REFRESHING'} - ${false} | ${'asdf'} | ${'BUILD_READY'} | ${'CONTENT_STALE_READY'} - ${false} | ${'asdf'} | ${'BUILD_TIMED_OUT'} | ${'CONTENT_STALE_TIMEOUT'} - ${false} | ${'asdf'} | ${'UP_TO_DATE'} | ${'CONTENT_FRESH'} - ${false} | ${'asdf'} | ${'ERROR'} | ${'CONTENT_STALE_ERROR'} + contentLoading | content | activeSyncState | expected + ${true} | ${''} | ${''} | ${'CHECKING'} + ${false} | ${undefined} | ${'CHECKING'} | ${'CHECKING'} + ${false} | ${undefined} | ${'BUILDING'} | ${'INITIAL_BUILD'} + ${false} | ${undefined} | ${'BUILD_READY'} | ${'CONTENT_NOT_FOUND'} + ${false} | ${undefined} | ${'BUILD_READY_RELOAD'} | ${'CHECKING'} + ${false} | ${undefined} | ${'BUILD_TIMED_OUT'} | ${'CONTENT_NOT_FOUND'} + ${false} | ${undefined} | ${'UP_TO_DATE'} | ${'CONTENT_NOT_FOUND'} + ${false} | ${undefined} | ${'ERROR'} | ${'CONTENT_NOT_FOUND'} + ${false} | ${'asdf'} | ${'CHECKING'} | ${'CONTENT_FRESH'} + ${false} | ${'asdf'} | ${'BUILDING'} | ${'CONTENT_STALE_REFRESHING'} + ${false} | ${'asdf'} | ${'BUILD_READY'} | ${'CONTENT_STALE_READY'} + ${false} | ${'asdf'} | ${'BUILD_READY_RELOAD'} | ${'CHECKING'} + ${false} | ${'asdf'} | ${'BUILD_TIMED_OUT'} | ${'CONTENT_STALE_TIMEOUT'} + ${false} | ${'asdf'} | ${'UP_TO_DATE'} | ${'CONTENT_FRESH'} + ${false} | ${'asdf'} | ${'ERROR'} | ${'CONTENT_STALE_ERROR'} `( 'should, when contentLoading=$contentLoading and content="$content" and activeSyncState=$activeSyncState, resolve to $expected', ({ contentLoading, content, activeSyncState, expected }) => { @@ -78,48 +80,80 @@ describe('useReaderState', () => { }); describe('reducer', () => { - const contentReloadFn = jest.fn(); const oldState: Parameters[0] = { activeSyncState: 'CHECKING', - contentIsStale: false, contentLoading: false, path: '', - contentReload: contentReloadFn, }; it('should return a copy of the state', () => { expect(reducer(oldState, { type: 'navigate', path: '/' })).toEqual({ activeSyncState: 'CHECKING', - contentIsStale: false, contentLoading: false, path: '/', - contentReload: contentReloadFn, }); expect(oldState).toEqual({ activeSyncState: 'CHECKING', - contentIsStale: false, contentLoading: false, path: '', - contentReload: contentReloadFn, }); }); - describe('"content" action', () => { - it('should work', () => { + 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} + `( + 'should, when type=$type and activeSyncState=$oldActiveSyncState, set activeSyncState=$newActiveSyncState', + ({ type, oldActiveSyncState, newActiveSyncState }) => { expect( reducer( { ...oldState, - content: undefined, + activeSyncState: oldActiveSyncState, + }, + { type }, + ).activeSyncState, + ).toEqual(newActiveSyncState); + }, + ); + + describe('"content" action', () => { + it('should set loading', () => { + expect( + reducer( + { + ...oldState, + content: 'some-old-content', + contentError: new Error(), + }, + { + type: 'content', contentLoading: true, - contentReload: undefined, + }, + ), + ).toEqual({ + ...oldState, + contentLoading: true, + }); + }); + + it('should set content', () => { + expect( + reducer( + { + ...oldState, + contentLoading: true, + contentError: new Error(), }, { type: 'content', content: 'asdf', - contentLoading: false, - contentReload: contentReloadFn, }, ), ).toEqual({ @@ -127,30 +161,25 @@ describe('useReaderState', () => { contentLoading: false, content: 'asdf', }); - - expect(contentReloadFn).toBeCalledTimes(0); }); - it('should reset staleness', () => { + it('should set error', () => { expect( reducer( { ...oldState, - contentIsStale: true, - activeSyncState: 'BUILD_READY', + contentLoading: true, + content: 'asdf', }, { type: 'content', - content: 'asdf', - contentLoading: false, - contentReload: contentReloadFn, + contentError: new Error(), }, ), ).toEqual({ ...oldState, - content: 'asdf', - contentIsStale: false, - activeSyncState: 'UP_TO_DATE', + contentLoading: false, + contentError: new Error(), }); }); }); @@ -166,28 +195,6 @@ describe('useReaderState', () => { ...oldState, path: '/', }); - - expect(contentReloadFn).toBeCalledTimes(0); - }); - - it('should reset staleness', () => { - expect( - reducer( - { - ...oldState, - contentIsStale: true, - activeSyncState: 'BUILD_READY', - }, - { - type: 'navigate', - path: '', - }, - ), - ).toEqual({ - ...oldState, - contentIsStale: false, - activeSyncState: 'UP_TO_DATE', - }); }); }); @@ -202,88 +209,6 @@ describe('useReaderState', () => { ...oldState, activeSyncState: 'BUILDING', }); - - expect(contentReloadFn).toBeCalledTimes(0); - }); - - it('should set content to be stale but not reload', () => { - expect( - reducer( - { - ...oldState, - contentReload: undefined, - }, - { - type: 'sync', - state: 'BUILD_READY', - }, - ), - ).toEqual({ - ...oldState, - activeSyncState: 'BUILD_READY', - contentIsStale: true, - contentReload: undefined, - }); - - expect(contentReloadFn).toBeCalledTimes(0); - }); - - it('should not reload existing content', () => { - expect( - reducer( - { - ...oldState, - content: 'any content', - }, - { - type: 'sync', - state: 'BUILD_READY', - }, - ), - ).toEqual({ - ...oldState, - activeSyncState: 'BUILD_READY', - contentIsStale: true, - content: 'any content', - }); - - expect(contentReloadFn).toBeCalledTimes(0); - }); - - it('should trigger a reload', () => { - expect( - reducer(oldState, { - type: 'sync', - state: 'BUILD_READY', - }), - ).toEqual({ - ...oldState, - activeSyncState: 'BUILD_READY', - contentIsStale: true, - contentLoading: true, - }); - - expect(contentReloadFn).toBeCalledTimes(1); - }); - - it('should NOT reset staleness', () => { - expect( - reducer( - { - ...oldState, - contentIsStale: true, - activeSyncState: 'BUILD_READY', - }, - { - type: 'sync', - state: 'BUILDING', - }, - ), - ).toEqual({ - ...oldState, - contentIsStale: true, - activeSyncState: 'BUILDING', - }); }); }); }); @@ -327,6 +252,68 @@ describe('useReaderState', () => { }); }); + it('should reload initially missing content', async () => { + techdocsStorageApi.getEntityDocs + .mockRejectedValueOnce(new NotFoundError('Page Not Found')) + .mockImplementationOnce(async () => { + await new Promise(resolve => setTimeout(resolve, 500)); + return 'my content'; + }); + techdocsStorageApi.syncEntityDocs.mockImplementation(async () => { + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'updated'; + }); + + await act(async () => { + const { result, waitForValueToChange } = await renderHook( + () => useReaderState('Component', 'default', 'backstage', '/example'), + { wrapper: Wrapper }, + ); + + expect(result.current).toEqual({ + state: 'CHECKING', + content: undefined, + errorMessage: '', + }); + + await waitForValueToChange(() => result.current.state); + + expect(result.current).toEqual({ + state: 'INITIAL_BUILD', + content: undefined, + errorMessage: ' Load error: NotFoundError: Page Not Found', + }); + + await waitForValueToChange(() => result.current.state); + + expect(result.current).toEqual({ + state: 'CHECKING', + content: undefined, + errorMessage: '', + }); + + await waitForValueToChange(() => result.current.state); + + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + content: 'my content', + errorMessage: '', + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledTimes(2); + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledTimes(1); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ + kind: 'Component', + namespace: 'default', + name: 'backstage', + }); + }); + }); + it('should handle stale content', async () => { techdocsStorageApi.getEntityDocs.mockResolvedValue('my content'); techdocsStorageApi.syncEntityDocs.mockImplementation(async () => { diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index f55e7c26e9..1dc4bc2677 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'; -import { useEffect, useMemo, useReducer } from 'react'; +import { useEffect, useMemo, useReducer, useRef } from 'react'; import { useAsync, useAsyncRetry } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; @@ -63,6 +63,11 @@ export function calculateDisplayState({ return 'CHECKING'; } + // the build is ready, but it triggered a content reload and the content variable is not trusted + if (activeSyncState === 'BUILD_READY_RELOAD') { + return 'CHECKING'; + } + // there is no content, but the sync process is still evaluating if (!content && activeSyncState === 'CHECKING') { return 'CHECKING'; @@ -116,6 +121,12 @@ type SyncStates = /** Finished building the documentation */ | 'BUILD_READY' + /** + * Finished building the documentation and triggered a content reload. + * This state is left toward UP_TO_DATE when the content loading has finished. + */ + | 'BUILD_READY_RELOAD' + /** Building the documentation timed out */ | 'BUILD_TIMED_OUT' @@ -134,9 +145,8 @@ type ReducerActions = | { type: 'content'; content?: string; - contentLoading: boolean; + contentLoading?: true; contentError?: Error; - contentReload: () => void; } | { type: 'navigate'; path: string }; @@ -159,14 +169,6 @@ type ReducerState = { * The content that has been downloaded and should be displayed. */ content?: string; - /** - * When called, the content is reloaded without refreshing the page. - */ - contentReload?: () => void; - /** - * If true, the content is considered stale and should be refreshed by the user via a refresh or a navigation. - */ - contentIsStale: boolean; contentError?: Error; syncError?: Error; @@ -182,27 +184,11 @@ export function reducer( case 'sync': newState.activeSyncState = action.state; newState.syncError = action.syncError; - - // whatever is stored as content, it can be considered as being stale - if (newState.activeSyncState === 'BUILD_READY') { - newState.contentIsStale = true; - - // reload the content if this was the initial build OR the page was missing in the old version - if (!newState.content && newState.contentReload) { - newState.contentReload(); - - // eagerly mark the content to load to not get synchronization issues since - // the async hook behind contentReload() doesn't update the reducer instantly - // and might flash the "not found" page - newState.contentLoading = true; - } - } break; case 'content': newState.content = action.content; - newState.contentLoading = action.contentLoading; - newState.contentReload = action.contentReload; + newState.contentLoading = action.contentLoading ?? false; newState.contentError = action.contentError; break; @@ -214,12 +200,11 @@ export function reducer( throw new Error(); } - // a navigation or a content update removes the staleness and resets the sync state + // a navigation or a content update loads fresh content so the build is updated to being up-to-date if ( - newState.contentIsStale && + ['BUILD_READY', 'BUILD_READY_RELOAD'].includes(newState.activeSyncState) && ['content', 'navigate'].includes(action.type) ) { - newState.contentIsStale = false; newState.activeSyncState = 'UP_TO_DATE'; } @@ -236,7 +221,6 @@ export function useReaderState( activeSyncState: 'CHECKING', path, contentLoading: true, - contentIsStale: false, }); const techdocsStorageApi = useApi(techdocsStorageApiRef); @@ -246,35 +230,33 @@ export function useReaderState( dispatch({ type: 'navigate', path }); }, [path]); - // try to load the content - const { - value: content, - loading: contentLoading, - error: contentError, - retry: contentReload, - } = useAsyncRetry( - async () => - techdocsStorageApi.getEntityDocs( - { - kind, - namespace, - name, - }, - path, - ), - [techdocsStorageApi, kind, namespace, name, 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 }); - // convert all content changes into actions - useEffect(() => { - dispatch({ - type: 'content', - content, - contentLoading, - contentReload, - contentError, - }); - }, [dispatch, content, contentLoading, contentReload, contentError]); + try { + const entityDocs = await techdocsStorageApi.getEntityDocs( + { kind, namespace, name }, + path, + ); + + dispatch({ type: 'content', content: entityDocs }); + + return entityDocs; + } catch (e) { + dispatch({ type: 'content', contentError: e }); + } + + return undefined; + }, [techdocsStorageApi, kind, namespace, name, path]); + + // create a ref that holds the latest content. This provides a useAsync hook + // with the latest content without restarting the useAsync hook. + const contentRef = useRef<{ content?: string; reload: () => void }>({ + content: undefined, + reload: () => {}, + }); + contentRef.current = { content: state.content, reload: contentReload }; // try to derive the state. the function will fire events and we don't care for the return values useAsync(async () => { @@ -293,7 +275,13 @@ export function useReaderState( }); if (result === 'updated') { - dispatch({ type: 'sync', state: 'BUILD_READY' }); + // if there was no content prior to building, retry the loading + if (!contentRef.current.content) { + contentRef.current.reload(); + dispatch({ type: 'sync', state: 'BUILD_READY_RELOAD' }); + } else { + dispatch({ type: 'sync', state: 'BUILD_READY' }); + } } else if (result === 'cached') { dispatch({ type: 'sync', state: 'UP_TO_DATE' }); } else { @@ -305,7 +293,7 @@ export function useReaderState( // Cancel the timer that sets the state "BUILDING" clearTimeout(buildingTimeout); } - }, [kind, name, namespace, techdocsStorageApi, dispatch]); + }, [kind, name, namespace, techdocsStorageApi, dispatch, contentRef]); const displayState = useMemo( () => @@ -329,7 +317,7 @@ export function useReaderState( return { state: displayState, - content, + content: state.content, errorMessage, }; }