From fea7fa0ba6e0d38e4a08747426fb6a7509912159 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 9 Jun 2021 10:22:56 +0200 Subject: [PATCH 1/4] Return a `304 Not Modified` from the `/sync/:namespace/:kind/:name` endpoint if nothing was built Signed-off-by: Dominik Henneke --- .changeset/techdocs-poor-forks-repeat.md | 5 +++++ plugins/techdocs-backend/src/DocsBuilder/builder.ts | 10 ++++++++-- plugins/techdocs-backend/src/service/router.ts | 13 ++++++++++--- 3 files changed, 23 insertions(+), 5 deletions(-) create mode 100644 .changeset/techdocs-poor-forks-repeat.md diff --git a/.changeset/techdocs-poor-forks-repeat.md b/.changeset/techdocs-poor-forks-repeat.md new file mode 100644 index 0000000000..89c4e0dc5a --- /dev/null +++ b/.changeset/techdocs-poor-forks-repeat.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs-backend': patch +--- + +Return a `304 Not Modified` from the `/sync/:namespace/:kind/:name` endpoint if nothing was built. This enables the caller to know whether a refresh of the docs page will return updated content (-> `201 Created`) or not (-> `304 Not Modified`). diff --git a/plugins/techdocs-backend/src/DocsBuilder/builder.ts b/plugins/techdocs-backend/src/DocsBuilder/builder.ts index dce391ff3f..24549f8b6f 100644 --- a/plugins/techdocs-backend/src/DocsBuilder/builder.ts +++ b/plugins/techdocs-backend/src/DocsBuilder/builder.ts @@ -68,7 +68,11 @@ export class DocsBuilder { this.config = config; } - public async build(): Promise { + /** + * Build the docs and return whether they have been newly generated or have been cached + * @returns true, if the docs have been built. false, if the cached docs are still up-to-date. + */ + public async build(): Promise { if (!this.entity.metadata.uid) { throw new Error( 'Trying to build documentation for entity not in service catalog', @@ -125,7 +129,7 @@ export class DocsBuilder { this.entity, )} are unmodified. Using cache, skipping generate and prepare`, ); - return; + return false; } throw new Error(err.message); } @@ -205,5 +209,7 @@ export class DocsBuilder { // Update the last check time for the entity new BuildMetadataStorage(this.entity.metadata.uid).setLastUpdated(); + + return true; } } diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index 1cd4075da0..1a8bdb9c8f 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -16,7 +16,7 @@ import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; -import { NotFoundError } from '@backstage/errors'; +import { NotFoundError, NotModifiedError } from '@backstage/errors'; import { GeneratorBuilder, getLocationForEntity, @@ -172,10 +172,10 @@ export async function createRouter({ case 'awsS3': case 'azureBlobStorage': case 'openStackSwift': - case 'googleGcs': + case 'googleGcs': { // This block should be valid for all storage implementations. So no need to duplicate in future, // add the publisher type in the list here. - await docsBuilder.build(); + const updated = await docsBuilder.build(); // With a maximum of ~5 seconds wait, check if the files got published and if docs will be fetched // on the user's page. If not, respond with a message asking them to check back later. // The delay here is to make sure GCS/AWS/etc. registers newly uploaded files which is usually <1 second @@ -194,10 +194,17 @@ export async function createRouter({ 'Sorry! It took too long for the generated docs to show up in storage. Check back later.', ); } + + if (!updated) { + throw new NotModifiedError(); + } + res .status(201) .json({ message: 'Docs updated or did not need updating' }); break; + } + default: throw new NotFoundError( `Publisher type ${publisherType} is not supported by techdocs-backend docs builder.`, From 1dfec7a2ae788d8b9c18a2ad278c11fa58c6bf5c Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 9 Jun 2021 10:49:10 +0200 Subject: [PATCH 2/4] Refactor the implicit logic from `` into an explicit state machine Signed-off-by: Dominik Henneke --- .changeset/techdocs-cool-rivers-suffer.md | 5 + plugins/techdocs/dev/api.ts | 149 ------ plugins/techdocs/dev/index.tsx | 193 +++++++- plugins/techdocs/package.json | 1 + plugins/techdocs/src/api.ts | 4 +- plugins/techdocs/src/client.ts | 14 +- .../techdocs/src/reader/components/Reader.tsx | 155 +++--- .../reader/components/useReaderState.test.tsx | 459 ++++++++++++++++++ .../src/reader/components/useReaderState.ts | 335 +++++++++++++ .../reader/transformers/addBaseUrl.test.ts | 2 +- 10 files changed, 1053 insertions(+), 264 deletions(-) create mode 100644 .changeset/techdocs-cool-rivers-suffer.md delete mode 100644 plugins/techdocs/dev/api.ts create mode 100644 plugins/techdocs/src/reader/components/useReaderState.test.tsx create mode 100644 plugins/techdocs/src/reader/components/useReaderState.ts diff --git a/.changeset/techdocs-cool-rivers-suffer.md b/.changeset/techdocs-cool-rivers-suffer.md new file mode 100644 index 0000000000..2386d27525 --- /dev/null +++ b/.changeset/techdocs-cool-rivers-suffer.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Refactor the implicit logic from `` into an explicit state machine. This resolves some state synchronization issues when content is refreshed or rebuilt in the backend. diff --git a/plugins/techdocs/dev/api.ts b/plugins/techdocs/dev/api.ts deleted file mode 100644 index e764bf82bb..0000000000 --- a/plugins/techdocs/dev/api.ts +++ /dev/null @@ -1,149 +0,0 @@ -/* - * Copyright 2020 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -import { EntityName } from '@backstage/catalog-model'; -import { Config } from '@backstage/config'; -import { DiscoveryApi, IdentityApi } from '@backstage/core'; -import { NotFoundError } from '@backstage/errors'; -import { TechDocsStorageApi } from '../src/api'; - -export class TechDocsDevStorageApi implements TechDocsStorageApi { - public configApi: Config; - public discoveryApi: DiscoveryApi; - public identityApi: IdentityApi; - - constructor({ - configApi, - discoveryApi, - identityApi, - }: { - configApi: Config; - discoveryApi: DiscoveryApi; - identityApi: IdentityApi; - }) { - this.configApi = configApi; - this.discoveryApi = discoveryApi; - this.identityApi = identityApi; - } - - async getApiOrigin() { - return ( - this.configApi.getOptionalString('techdocs.requestUrl') ?? - (await this.discoveryApi.getBaseUrl('techdocs')) - ); - } - - async getStorageUrl() { - return ( - this.configApi.getOptionalString('techdocs.storageUrl') ?? - `${await this.discoveryApi.getBaseUrl('techdocs')}/static/docs` - ); - } - - async getBuilder() { - return this.configApi.getString('techdocs.builder'); - } - - async fetchUrl(url: string) { - const token = await this.identityApi.getIdToken(); - return fetch(url, { - headers: token ? { Authorization: `Bearer ${token}` } : {}, - }); - } - - async getEntityDocs(entityId: EntityName, path: string) { - const { kind, namespace, name } = entityId; - - const storageUrl = await this.getStorageUrl(); - const url = `${storageUrl}/${namespace}/${kind}/${name}/${path}`; - const token = await this.identityApi.getIdToken(); - - const request = await fetch( - `${url.endsWith('/') ? url : `${url}/`}index.html`, - { - headers: token ? { Authorization: `Bearer ${token}` } : {}, - }, - ); - - let errorMessage = ''; - switch (request.status) { - case 404: - errorMessage = 'Page not found. '; - // path is empty for the home page of an entity's docs site - if (!path) { - errorMessage += - 'This could be because there is no index.md file in the root of the docs directory of this repository.'; - } - throw new NotFoundError(errorMessage); - case 500: - errorMessage = - 'Could not generate documentation or an error in the TechDocs backend. '; - throw new Error(errorMessage); - default: - // Do nothing - break; - } - - return request.text(); - } - - /** - * Check if docs are the latest version and trigger rebuilds if not - * - * @param {EntityName} entityId Object containing entity data like name, namespace, etc. - * @returns {boolean} Whether documents are currently synchronized to newest version - * @throws {Error} Throws error on error from sync endpoint - */ - async syncEntityDocs(entityId: EntityName) { - const { kind, namespace, name } = entityId; - - const apiOrigin = await this.getApiOrigin(); - const url = `${apiOrigin}/sync/${namespace}/${kind}/${name}`; - let request; - let attempts: number = 0; - // retry if request times out, up to 5 times - // can happen due to docs taking too long to generate - while (!request || (request.status === 408 && attempts < 5)) { - attempts++; - request = await this.fetchUrl( - `${url.endsWith('/') ? url : `${url}/`}index.html`, - ); - } - - switch (request.status) { - case 404: - throw (await request.json()).error; - case 200: - case 201: - return true; - // for timeout and misc errors, handle without error to allow viewing older docs - // if older docs not available, - // Reader will show 404 error coming from getEntityDocs - case 408: - default: - return false; - } - } - - async getBaseUrl( - oldBaseUrl: string, - entityId: EntityName, - path: string, - ): Promise { - const { name } = entityId; - const apiOrigin = await this.getApiOrigin(); - return new URL(oldBaseUrl, `${apiOrigin}/${name}/${path}`).toString(); - } -} diff --git a/plugins/techdocs/dev/index.tsx b/plugins/techdocs/dev/index.tsx index 3eefa0a814..a778161b35 100644 --- a/plugins/techdocs/dev/index.tsx +++ b/plugins/techdocs/dev/index.tsx @@ -14,11 +14,105 @@ * limitations under the License. */ -import { configApiRef, discoveryApiRef, identityApiRef } from '@backstage/core'; +import { + configApiRef, + discoveryApiRef, + Header, + identityApiRef, + Page, + TabbedLayout, +} from '@backstage/core'; import { createDevApp } from '@backstage/dev-utils'; -import { techdocsPlugin } from '../src/plugin'; -import { TechDocsDevStorageApi } from './api'; -import { techdocsStorageApiRef } from '../src'; +import { NotFoundError } from '@backstage/errors'; +import React from 'react'; +import { + Reader, + SyncResult, + TechDocsStorageApi, + techdocsStorageApiRef, +} from '../src'; + +// used so each route can provide it's own implementation in the constructor of the react component +let apiHolder: TechDocsStorageApi | undefined = undefined; + +const apiBridge: TechDocsStorageApi = { + getApiOrigin: async () => '', + getBaseUrl: (...args) => apiHolder!.getBaseUrl(...args), + getBuilder: () => apiHolder!.getBuilder(), + getStorageUrl: () => apiHolder!.getStorageUrl(), + getEntityDocs: (...args) => apiHolder!.getEntityDocs(...args), + syncEntityDocs: (...args) => apiHolder!.syncEntityDocs(...args), +}; + +const mockContent = ` +

Hello World!

+

This is an example content that will actually be provided by a MkDocs powered site

+`; + +function createPage({ + entityDocs, + syncDocs, + syncDocsDelay, +}: { + entityDocs?: (props: { + called: number; + content: string; + }) => string | Promise; + syncDocs: () => SyncResult; + syncDocsDelay?: number; +}) { + class Api implements TechDocsStorageApi { + private entityDocsCallCount: number = 0; + + getApiOrigin = async () => ''; + getBaseUrl = async () => ''; + getBuilder = async () => 'local'; + getStorageUrl = async () => ''; + + async getEntityDocs() { + await new Promise(resolve => setTimeout(resolve, 500)); + + if (!entityDocs) { + return mockContent; + } + + return entityDocs({ + called: this.entityDocsCallCount++, + content: mockContent, + }); + } + + async syncEntityDocs() { + if (syncDocsDelay) { + await new Promise(resolve => setTimeout(resolve, syncDocsDelay)); + } + + return syncDocs(); + } + } + + class Component extends React.Component { + constructor(props: {}) { + super(props); + + apiHolder = new Api(); + } + + render() { + return ( + + ); + } + } + + return ; +} createDevApp() .registerApi({ @@ -28,12 +122,89 @@ createDevApp() discoveryApi: discoveryApiRef, identityApi: identityApiRef, }, - factory: ({ configApi, discoveryApi, identityApi }) => - new TechDocsDevStorageApi({ - configApi, - discoveryApi, - identityApi, - }), + factory: () => apiBridge, + }) + + .addPage({ + title: 'TechDocs', + element: ( + +
+ + + {createPage({ + syncDocs: () => 'cached', + })} + + + + {createPage({ + syncDocs: () => 'updated', + syncDocsDelay: 2000, + })} + + + + {createPage({ + entityDocs: ({ called, content }) => { + if (called < 1) { + throw new NotFoundError(); + } + + return content; + }, + syncDocs: () => 'updated', + syncDocsDelay: 10000, + })} + + + + {createPage({ + entityDocs: () => { + throw new NotFoundError('Not found, some error message...'); + }, + syncDocs: () => 'cached', + })} + + + + {createPage({ + entityDocs: () => { + throw new Error('Another more critical error'); + }, + syncDocs: () => 'cached', + })} + + + + {createPage({ + syncDocs: () => { + throw new Error('Some random error'); + }, + syncDocsDelay: 2000, + })} + + + + {createPage({ + entityDocs: () => { + throw new Error('Some random error'); + }, + syncDocs: () => { + throw new Error('Some random error'); + }, + syncDocsDelay: 2000, + })} + + + + {createPage({ + syncDocs: () => 'timeout', + syncDocsDelay: 2000, + })} + + + + ), }) - .registerPlugin(techdocsPlugin) .render(); diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 76bcfd8b0d..c3336ac3b2 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -56,6 +56,7 @@ "@backstage/test-utils": "^0.1.13", "@testing-library/jest-dom": "^5.10.1", "@testing-library/react": "^11.2.5", + "@testing-library/react-hooks": "^3.4.2", "@testing-library/user-event": "^13.1.8", "@types/react": "^16.9", "@types/jest": "^26.0.7", diff --git a/plugins/techdocs/src/api.ts b/plugins/techdocs/src/api.ts index 204cc1b5a8..b9c8725ce9 100644 --- a/plugins/techdocs/src/api.ts +++ b/plugins/techdocs/src/api.ts @@ -28,12 +28,14 @@ export const techdocsApiRef = createApiRef({ description: 'Used to make requests towards techdocs API', }); +export type SyncResult = 'cached' | 'updated' | 'timeout'; + export interface TechDocsStorageApi { getApiOrigin(): Promise; getStorageUrl(): Promise; getBuilder(): Promise; getEntityDocs(entityId: EntityName, path: string): Promise; - syncEntityDocs(entityId: EntityName): Promise; + syncEntityDocs(entityId: EntityName): Promise; getBaseUrl( oldBaseUrl: string, entityId: EntityName, diff --git a/plugins/techdocs/src/client.ts b/plugins/techdocs/src/client.ts index 245cfb0154..16617dcbce 100644 --- a/plugins/techdocs/src/client.ts +++ b/plugins/techdocs/src/client.ts @@ -18,7 +18,7 @@ import { EntityName } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { DiscoveryApi, IdentityApi } from '@backstage/core'; import { NotFoundError } from '@backstage/errors'; -import { TechDocsApi, TechDocsStorageApi } from './api'; +import { SyncResult, TechDocsApi, TechDocsStorageApi } from './api'; import { TechDocsEntityMetadata, TechDocsMetadata } from './types'; /** @@ -195,7 +195,7 @@ export class TechDocsStorageClient implements TechDocsStorageApi { * @returns {boolean} Whether documents are currently synchronized to newest version * @throws {Error} Throws error on error from sync endpoint in Techdocs Backend */ - async syncEntityDocs(entityId: EntityName): Promise { + async syncEntityDocs(entityId: EntityName): Promise { const { kind, namespace, name } = entityId; const apiOrigin = await this.getApiOrigin(); @@ -215,16 +215,20 @@ export class TechDocsStorageClient implements TechDocsStorageApi { switch (request.status) { case 404: throw new NotFoundError((await request.json()).error); + case 200: - case 201: case 304: - return true; + return 'cached'; + + case 201: + return 'updated'; + // for timeout and misc errors, handle without error to allow viewing older docs // if older docs not available, // Reader will show 404 error coming from getEntityDocs case 408: default: - return false; + return 'timeout'; } } diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index c779985364..75dade20fa 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -13,15 +13,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { EntityName } from '@backstage/catalog-model'; import { useApi } from '@backstage/core'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; import { BackstageTheme } from '@backstage/theme'; import { useTheme } from '@material-ui/core'; import { Alert } from '@material-ui/lab'; -import React, { useEffect, useRef, useState } from 'react'; +import React, { useCallback, useEffect, useRef, useState } from 'react'; import { useNavigate, useParams } from 'react-router-dom'; -import { useAsync } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; import { addBaseUrl, @@ -37,7 +37,7 @@ import { } from '../transformers'; import { TechDocsNotFound } from './TechDocsNotFound'; import TechDocsProgressBar from './TechDocsProgressBar'; -import { useRawPage } from './useRawPage'; +import { useReaderState } from './useReaderState'; type Props = { entityId: EntityName; @@ -49,61 +49,37 @@ export const Reader = ({ entityId, onReady }: Props) => { const { '*': path } = useParams(); const theme = useTheme(); + const { state, content: rawPage, errorMessage } = useReaderState( + kind, + namespace, + name, + path, + ); + const techdocsStorageApi = useApi(techdocsStorageApiRef); const [sidebars, setSidebars] = useState(); const navigate = useNavigate(); const shadowDomRef = useRef(null); - const [loadedPath, setLoadedPath] = useState(''); - const [atInitialLoad, setAtInitialLoad] = useState(true); - const [newerDocsExist, setNewerDocsExist] = useState(false); const scmIntegrationsApi = useApi(scmIntegrationsApiRef); - const { - value: isSynced, - loading: syncInProgress, - error: syncError, - } = useAsync(async () => { - // Attempt to sync only if `techdocs.builder` in app config is set to 'local' - if ((await techdocsStorageApi.getBuilder()) !== 'local') { - return Promise.resolve({ - value: true, - loading: null, - error: null, + const updateSidebarPosition = useCallback(() => { + if (!!shadowDomRef.current && !!sidebars) { + const mdTabs = shadowDomRef.current!.querySelector( + '.md-container > .md-tabs', + ); + sidebars!.forEach(sidebar => { + const newTop = Math.max( + shadowDomRef.current!.getBoundingClientRect().top, + 0, + ); + sidebar.style.top = mdTabs + ? `${newTop + mdTabs.getBoundingClientRect().height}px` + : `${newTop}px`; }); } - return techdocsStorageApi.syncEntityDocs({ kind, namespace, name }); - }, [techdocsStorageApi, kind, namespace, name]); - - const { - value: rawPage, - loading: docLoading, - error: docLoadError, - retry, - } = useRawPage(path, kind, namespace, name); + }, [shadowDomRef, sidebars]); useEffect(() => { - if (isSynced && newerDocsExist && path !== loadedPath) { - retry(); - } - }); - - useEffect(() => { - const updateSidebarPosition = () => { - if (!!shadowDomRef.current && !!sidebars) { - const mdTabs = shadowDomRef.current!.querySelector( - '.md-container > .md-tabs', - ); - sidebars!.forEach(sidebar => { - const newTop = Math.max( - shadowDomRef.current!.getBoundingClientRect().top, - 0, - ); - sidebar.style.top = mdTabs - ? `${newTop + mdTabs.getBoundingClientRect().height}px` - : `${newTop}px`; - }); - } - }; updateSidebarPosition(); window.addEventListener('scroll', updateSidebarPosition); window.addEventListener('resize', updateSidebarPosition); @@ -111,28 +87,8 @@ export const Reader = ({ entityId, onReady }: Props) => { window.removeEventListener('scroll', updateSidebarPosition); window.removeEventListener('resize', updateSidebarPosition); }; - }, [shadowDomRef, sidebars]); - - useEffect(() => { - if (rawPage) { - setLoadedPath(path); - } - }, [rawPage, path]); - - useEffect(() => { - if (atInitialLoad === false) { - return; - } - setTimeout(() => { - setAtInitialLoad(false); - }, 5000); - }); - - useEffect(() => { - if (!atInitialLoad && !!rawPage && syncInProgress) { - setNewerDocsExist(true); - } - }, [atInitialLoad, rawPage, syncInProgress]); + // an update to "state" might lead to an updated UI so we include it as a trigger + }, [updateSidebarPosition, state]); useEffect(() => { if (!rawPage || !shadowDomRef.current) { @@ -142,12 +98,16 @@ export const Reader = ({ entityId, onReady }: Props) => { onReady(); } // Pre-render - const transformedElement = transformer(rawPage.content, [ + const transformedElement = transformer(rawPage, [ sanitizeDOM(), addBaseUrl({ techdocsStorageApi, - entityId: rawPage.entityId, - path: rawPage.path, + entityId: { + kind, + name, + namespace, + }, + path, }), rewriteDocLinks(), removeMkdocsHeader(), @@ -292,10 +252,6 @@ export const Reader = ({ entityId, onReady }: Props) => { baseUrl: window.location.origin, onClick: (_: MouseEvent, url: string) => { const parsedUrl = new URL(url); - if (newerDocsExist && isSynced) { - // link navigation will load newer docs - setNewerDocsExist(false); - } if (parsedUrl.hash) { navigate(`${parsedUrl.pathname}${parsedUrl.hash}`); @@ -337,6 +293,10 @@ export const Reader = ({ entityId, onReady }: Props) => { }), ]); }, [ + path, + kind, + namespace, + name, rawPage, navigate, onReady, @@ -347,39 +307,40 @@ export const Reader = ({ entityId, onReady }: Props) => { theme.palette.primary.main, theme.palette.background.paper, theme.palette.background.default, - newerDocsExist, - isSynced, scmIntegrationsApi, ]); - // docLoadError not considered an error state if sync request is still ongoing - // or sync just completed and doc is loading again - if ((docLoadError && !syncInProgress && !docLoading) || syncError) { - let errMessage = ''; - if (docLoadError) { - errMessage += ` Load error: ${docLoadError}`; - } - if (syncError) errMessage += ` Build error: ${syncError}`; - return ; - } - return ( <> - {newerDocsExist && !isSynced ? ( + {(state === 'CHECKING' || state === 'INITIAL_BUILD') && ( + + )} + {state === 'CONTENT_STALE_REFRESHING' && ( A newer version of this documentation is being prepared and will be available shortly. - ) : null} - {newerDocsExist && isSynced ? ( + )} + {state === 'CONTENT_STALE_READY' && ( A newer version of this documentation is now available, please refresh to view. - ) : null} - {docLoading || (docLoadError && syncInProgress) ? ( - - ) : null} + )} + {state === 'CONTENT_STALE_TIMEOUT' && ( + + Building a newer version of this documentation took longer than + expected. Please refresh to try again. + + )} + {state === 'CONTENT_STALE_ERROR' && ( + + Building a newer version of this documentation failed. {errorMessage} + + )} + {state === 'CONTENT_NOT_FOUND' && ( + + )}
); diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx new file mode 100644 index 0000000000..d5579ddd5a --- /dev/null +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -0,0 +1,459 @@ +/* + * Copyright 2021 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ApiProvider, ApiRegistry } from '@backstage/core'; +import { NotFoundError } from '@backstage/errors'; +import { act, renderHook } from '@testing-library/react-hooks'; +import React from 'react'; +import { techdocsStorageApiRef } from '../../api'; +import { + calculateDisplayState, + reducer, + useReaderState, +} from './useReaderState'; + +describe('useReaderState', () => { + let Wrapper: React.ComponentType; + + const techdocsStorageApi: jest.Mocked = { + getApiOrigin: jest.fn(), + getBaseUrl: jest.fn(), + getBuilder: jest.fn(), + getEntityDocs: jest.fn(), + getStorageUrl: jest.fn(), + syncEntityDocs: jest.fn(), + }; + + beforeEach(() => { + const apis = ApiRegistry.with(techdocsStorageApiRef, techdocsStorageApi); + + Wrapper = ({ children }: { children?: React.ReactNode }) => ( + {children} + ); + }); + + afterEach(() => jest.resetAllMocks()); + + 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'} + `( + 'should, when contentLoading=$contentLoading and content="$content" and activeSyncState=$activeSyncState, resolve to $expected', + ({ contentLoading, content, activeSyncState, expected }) => { + expect( + calculateDisplayState({ + contentLoading, + content, + activeSyncState, + }), + ).toEqual(expected); + }, + ); + }); + + 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', () => { + expect( + reducer( + { + ...oldState, + content: undefined, + contentLoading: true, + contentReload: undefined, + }, + { + type: 'content', + content: 'asdf', + contentLoading: false, + contentReload: contentReloadFn, + }, + ), + ).toEqual({ + ...oldState, + contentLoading: false, + content: 'asdf', + }); + + expect(contentReloadFn).toBeCalledTimes(0); + }); + + it('should reset staleness', () => { + expect( + reducer( + { + ...oldState, + contentIsStale: true, + activeSyncState: 'BUILD_READY', + }, + { + type: 'content', + content: 'asdf', + contentLoading: false, + contentReload: contentReloadFn, + }, + ), + ).toEqual({ + ...oldState, + content: 'asdf', + contentIsStale: false, + activeSyncState: 'UP_TO_DATE', + }); + }); + }); + + describe('"navigate" action', () => { + it('should work', () => { + expect( + reducer(oldState, { + type: 'navigate', + path: '/', + }), + ).toEqual({ + ...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', + }); + }); + }); + + describe('"sync" action', () => { + it('should update state', () => { + expect( + reducer(oldState, { + type: 'sync', + state: 'BUILDING', + }), + ).toEqual({ + ...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', + }); + }); + }); + }); + + describe('hook', () => { + it('should handle up-to-date content', async () => { + techdocsStorageApi.getEntityDocs.mockResolvedValue('my content'); + techdocsStorageApi.syncEntityDocs.mockImplementation(async () => { + return 'cached'; + }); + + 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: 'CONTENT_FRESH', + content: 'my content', + errorMessage: '', + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + 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 () => { + 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: '', + }); + + // the content is returned but the sync is in progress + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + content: 'my content', + errorMessage: '', + }); + + // the sync takes longer than 1 seconds so the refreshing state starts + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_STALE_REFRESHING', + content: 'my content', + errorMessage: '', + }); + + // the content is up-to-date + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_STALE_READY', + content: 'my content', + errorMessage: '', + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ + kind: 'Component', + namespace: 'default', + name: 'backstage', + }); + }); + }); + + it('should handle timed-out refresh', async () => { + techdocsStorageApi.getEntityDocs.mockResolvedValue('my content'); + techdocsStorageApi.syncEntityDocs.mockResolvedValue('timeout'); + + await act(async () => { + const { result, waitForValueToChange } = await renderHook( + () => useReaderState('Component', 'default', 'backstage', '/example'), + { wrapper: Wrapper }, + ); + + expect(result.current).toEqual({ + state: 'CHECKING', + content: undefined, + errorMessage: '', + }); + + // the content is returned but the sync is in progress + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_STALE_TIMEOUT', + content: 'my content', + errorMessage: '', + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ + kind: 'Component', + namespace: 'default', + name: 'backstage', + }); + }); + }); + + it('should handle content error', async () => { + techdocsStorageApi.getEntityDocs.mockRejectedValue( + new NotFoundError('Some error description'), + ); + techdocsStorageApi.syncEntityDocs.mockResolvedValue('cached'); + + await act(async () => { + const { result, waitForValueToChange } = await renderHook( + () => useReaderState('Component', 'default', 'backstage', '/example'), + { wrapper: Wrapper }, + ); + + expect(result.current).toEqual({ + state: 'CHECKING', + content: undefined, + errorMessage: '', + }); + + // the content loading threw an error + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_NOT_FOUND', + content: undefined, + errorMessage: ' Load error: NotFoundError: Some error description', + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ + kind: 'Component', + namespace: 'default', + name: 'backstage', + }); + }); + }); + }); +}); diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts new file mode 100644 index 0000000000..f55e7c26e9 --- /dev/null +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -0,0 +1,335 @@ +/* + * Copyright 2021 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { useApi } from '@backstage/core'; +import { useEffect, useMemo, useReducer } from 'react'; +import { useAsync, useAsyncRetry } from 'react-use'; +import { techdocsStorageApiRef } from '../../api'; + +/** + * A state representation that is used to configure the UI of + */ +type ContentStateTypes = + /** There is nothing to display but a loading indicator */ + | 'CHECKING' + + /** There is no content yet -> present a full screen loading page */ + | 'INITIAL_BUILD' + + /** There is content, but the backend is about to update it */ + | 'CONTENT_STALE_REFRESHING' + + /** There is content, but after a reload, the content will be different */ + | 'CONTENT_STALE_READY' + + /** There is content, the backend tried to update it, but it took too long */ + | 'CONTENT_STALE_TIMEOUT' + + /** There is content, the backend tried to update it, but failed */ + | 'CONTENT_STALE_ERROR' + + /** There is nothing to see but a "not found" page. Is also shown on page load errors */ + | 'CONTENT_NOT_FOUND' + + /** There is only the latest and greatest content */ + | 'CONTENT_FRESH'; + +/** + * Calculate the state that should be reported to the display component. + */ +export function calculateDisplayState({ + contentLoading, + content, + activeSyncState, +}: Pick< + ReducerState, + 'contentLoading' | 'content' | 'activeSyncState' +>): ContentStateTypes { + // we have nothing to display yet + if (contentLoading) { + return 'CHECKING'; + } + + // there is no content, but the sync process is still evaluating + if (!content && activeSyncState === 'CHECKING') { + return 'CHECKING'; + } + + // there is no content yet so we assume that we are building it for the first time + if (!content && activeSyncState === 'BUILDING') { + return 'INITIAL_BUILD'; + } + + // if there is still no content after building, it might just not exist + if (!content) { + return 'CONTENT_NOT_FOUND'; + } + + // we are still building, but we already show stale content + if (activeSyncState === 'BUILDING') { + return 'CONTENT_STALE_REFRESHING'; + } + + // the build is ready, but the content is still stale + if (activeSyncState === 'BUILD_READY') { + return 'CONTENT_STALE_READY'; + } + + // the build timed out, but the content is still stale + if (activeSyncState === 'BUILD_TIMED_OUT') { + return 'CONTENT_STALE_TIMEOUT'; + } + + // the build failed, but the content is still stale + if (activeSyncState === 'ERROR') { + return 'CONTENT_STALE_ERROR'; + } + + // seems like the content is up-to-date (or we don't know yet and the sync process is still evaluating in the background) + return 'CONTENT_FRESH'; +} + +/** + * The state of the synchronization task. It checks whether the docs are + * up-to-date. If they aren't, it triggers a build. + */ +type SyncStates = + /** Checking if it should be synced */ + | 'CHECKING' + + /** Building the documentation */ + | 'BUILDING' + + /** Finished building the documentation */ + | 'BUILD_READY' + + /** Building the documentation timed out */ + | 'BUILD_TIMED_OUT' + + /** No need for a sync. The content was already up-to-date. */ + | 'UP_TO_DATE' + + /** An error occurred */ + | 'ERROR'; + +type ReducerActions = + | { + type: 'sync'; + state: SyncStates; + syncError?: Error; + } + | { + type: 'content'; + content?: string; + contentLoading: boolean; + contentError?: Error; + contentReload: () => void; + } + | { type: 'navigate'; path: string }; + +type ReducerState = { + /** + * The path of the current page + */ + path: string; + + /** + * The current sync state + */ + activeSyncState: SyncStates; + + /** + * If true, the content is downloading from the storage. + */ + contentLoading: boolean; + /** + * 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; +}; + +export function reducer( + oldState: ReducerState, + action: ReducerActions, +): ReducerState { + const newState = { ...oldState }; + + switch (action.type) { + 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.contentError = action.contentError; + break; + + case 'navigate': + newState.path = action.path; + break; + + default: + throw new Error(); + } + + // a navigation or a content update removes the staleness and resets the sync state + if ( + newState.contentIsStale && + ['content', 'navigate'].includes(action.type) + ) { + newState.contentIsStale = false; + newState.activeSyncState = 'UP_TO_DATE'; + } + + return newState; +} + +export function useReaderState( + kind: string, + namespace: string, + name: string, + path: string, +): { state: ContentStateTypes; content?: string; errorMessage?: string } { + const [state, dispatch] = useReducer(reducer, { + activeSyncState: 'CHECKING', + path, + contentLoading: true, + contentIsStale: false, + }); + + const techdocsStorageApi = useApi(techdocsStorageApiRef); + + // convert all path changes into actions + useEffect(() => { + 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], + ); + + // convert all content changes into actions + useEffect(() => { + dispatch({ + type: 'content', + content, + contentLoading, + contentReload, + contentError, + }); + }, [dispatch, content, contentLoading, contentReload, contentError]); + + // try to derive the state. the function will fire events and we don't care for the return values + useAsync(async () => { + dispatch({ type: 'sync', state: 'CHECKING' }); + + // should only switch to BUILDING if the request takes more than 1 seconds + const buildingTimeout = setTimeout(() => { + dispatch({ type: 'sync', state: 'BUILDING' }); + }, 1000); + + try { + const result = await techdocsStorageApi.syncEntityDocs({ + kind, + namespace, + name, + }); + + if (result === 'updated') { + dispatch({ type: 'sync', state: 'BUILD_READY' }); + } else if (result === 'cached') { + dispatch({ type: 'sync', state: 'UP_TO_DATE' }); + } else { + dispatch({ type: 'sync', state: 'BUILD_TIMED_OUT' }); + } + } catch (e) { + dispatch({ type: 'sync', state: 'ERROR', syncError: e }); + } finally { + // Cancel the timer that sets the state "BUILDING" + clearTimeout(buildingTimeout); + } + }, [kind, name, namespace, techdocsStorageApi, dispatch]); + + const displayState = useMemo( + () => + calculateDisplayState({ + activeSyncState: state.activeSyncState, + contentLoading: state.contentLoading, + content: state.content, + }), + [state.activeSyncState, state.content, state.contentLoading], + ); + + const errorMessage = useMemo(() => { + let errMessage = ''; + if (state.contentError) { + errMessage += ` Load error: ${state.contentError}`; + } + if (state.syncError) errMessage += ` Build error: ${state.syncError}`; + + return errMessage; + }, [state.syncError, state.contentError]); + + return { + state: displayState, + content, + errorMessage, + }; +} diff --git a/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts b/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts index 6b6eae4b0d..9bfcbe624a 100644 --- a/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts +++ b/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts @@ -27,7 +27,7 @@ const techdocsStorageApi: TechDocsStorageApi = { Promise.resolve(new URL(o, DOC_STORAGE_URL).toString()), ), getEntityDocs: () => new Promise(resolve => resolve('yes!')), - syncEntityDocs: () => new Promise(resolve => resolve(true)), + syncEntityDocs: () => new Promise(resolve => resolve('updated')), getApiOrigin: jest.fn(() => new Promise(resolve => resolve(API_ORIGIN_URL))), getBuilder: jest.fn(), getStorageUrl: jest.fn(), From 2b9d30b1535f22a1d65fb11021cefb1c1baed916 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 16 Jun 2021 11:04:26 +0200 Subject: [PATCH 3/4] Fix minor issues from the review comments Signed-off-by: Dominik Henneke --- plugins/techdocs-backend/src/service/router.ts | 9 +++++---- plugins/techdocs/src/client.ts | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index 1a8bdb9c8f..ff47e16bc9 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -176,6 +176,11 @@ export async function createRouter({ // This block should be valid for all storage implementations. So no need to duplicate in future, // add the publisher type in the list here. const updated = await docsBuilder.build(); + + if (!updated) { + throw new NotModifiedError(); + } + // With a maximum of ~5 seconds wait, check if the files got published and if docs will be fetched // on the user's page. If not, respond with a message asking them to check back later. // The delay here is to make sure GCS/AWS/etc. registers newly uploaded files which is usually <1 second @@ -195,10 +200,6 @@ export async function createRouter({ ); } - if (!updated) { - throw new NotModifiedError(); - } - res .status(201) .json({ message: 'Docs updated or did not need updating' }); diff --git a/plugins/techdocs/src/client.ts b/plugins/techdocs/src/client.ts index 16617dcbce..83cfc88d56 100644 --- a/plugins/techdocs/src/client.ts +++ b/plugins/techdocs/src/client.ts @@ -192,7 +192,7 @@ export class TechDocsStorageClient implements TechDocsStorageApi { * Check if docs are on the latest version and trigger rebuild if not * * @param {EntityName} entityId Object containing entity data like name, namespace, etc. - * @returns {boolean} Whether documents are currently synchronized to newest version + * @returns {SyncResult} Whether documents are currently synchronized to newest version * @throws {Error} Throws error on error from sync endpoint in Techdocs Backend */ async syncEntityDocs(entityId: EntityName): Promise { From ba984f675a121df2260e3a7c717d59a8448cf219 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 16 Jun 2021 11:04:47 +0200 Subject: [PATCH 4/4] 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, }; }