From 06a5ddfefbc48af2080e83e488ba8d2c41e7ce0a Mon Sep 17 00:00:00 2001 From: Olga Bulat Date: Wed, 8 Jan 2025 13:22:20 +0300 Subject: [PATCH] Refactor FetchState to use discriminated union Fixes #5322 --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/WordPress/openverse/issues/5322?shareId=XXXX-XXXX-XXXX-XXXX). --- frontend/shared/types/fetch-state.ts | 9 +- frontend/shared/utils/errors.ts | 2 +- .../VCollectionHeader/VCollectionHeader.vue | 2 +- .../src/components/VHeader/VHeaderDesktop.vue | 2 +- .../src/components/VHeader/VHeaderMobile.vue | 405 ++++++++++++++++++ .../components/VMediaInfo/VRelatedMedia.vue | 2 +- frontend/src/composables/use-collection.ts | 2 +- frontend/src/composables/use-search.ts | 2 +- frontend/src/middleware/search.ts | 2 +- frontend/src/pages/search.vue | 4 +- frontend/src/stores/media/related-media.ts | 19 +- frontend/src/stores/media/single-result.ts | 16 +- frontend/src/stores/provider.ts | 9 +- .../specs/stores/media-store-fetching.spec.js | 8 +- .../unit/specs/stores/media-store.spec.ts | 54 +-- .../specs/stores/single-result-store.spec.ts | 8 +- 16 files changed, 463 insertions(+), 83 deletions(-) create mode 100644 frontend/src/components/VHeader/VHeaderMobile.vue diff --git a/frontend/shared/types/fetch-state.ts b/frontend/shared/types/fetch-state.ts index dd80cdc9e00..1fd06543b02 100644 --- a/frontend/shared/types/fetch-state.ts +++ b/frontend/shared/types/fetch-state.ts @@ -27,9 +27,6 @@ export interface FetchingError { details?: Record } -export interface FetchState { - isFetching: boolean - hasStarted?: boolean - isFinished?: boolean - fetchingError: FetchingError | null -} +export type FetchState = + | { status: 'idle' | 'fetching' | 'success'; error: null } + | { status: 'error'; error: FetchingError }; diff --git a/frontend/shared/utils/errors.ts b/frontend/shared/utils/errors.ts index 5f19d07c84b..36ab4b03870 100644 --- a/frontend/shared/utils/errors.ts +++ b/frontend/shared/utils/errors.ts @@ -16,7 +16,7 @@ const isNonRetryableErrorStatusCode = (statusCode: number | undefined) => { * TODO: Update this function with other error codes if needed. */ export const isRetriable = (error: FetchingError) => { - const { statusCode, code } = error + const { statusCode, code } = error.error return !(isNonRetryableErrorStatusCode(statusCode) || code === NO_RESULT) } diff --git a/frontend/src/components/VCollectionHeader/VCollectionHeader.vue b/frontend/src/components/VCollectionHeader/VCollectionHeader.vue index 637a3c0e81f..41560161b8f 100644 --- a/frontend/src/components/VCollectionHeader/VCollectionHeader.vue +++ b/frontend/src/components/VCollectionHeader/VCollectionHeader.vue @@ -92,7 +92,7 @@ const showCollectionExternalLink = computed(() => { const { getI18nCollectionResultCountLabel } = useI18nResultsCount() const resultsLabel = computed(() => { - if (mediaStore.resultCount === 0 && mediaStore.fetchState.isFetching) { + if (mediaStore.resultCount === 0 && mediaStore.fetchState.status === 'loading') { return "" } const resultsCount = mediaStore.results[props.mediaType].count diff --git a/frontend/src/components/VHeader/VHeaderDesktop.vue b/frontend/src/components/VHeader/VHeaderDesktop.vue index f2681c96ec8..1aea2aa8770 100644 --- a/frontend/src/components/VHeader/VHeaderDesktop.vue +++ b/frontend/src/components/VHeader/VHeaderDesktop.vue @@ -29,7 +29,7 @@ const uiStore = useUiStore() const isSidebarVisible = inject>(IsSidebarVisibleKey) -const isFetching = computed(() => mediaStore.fetchState.isFetching) +const isFetching = computed(() => mediaStore.fetchState.status === 'loading') const { $sendCustomEvent } = useNuxtApp() diff --git a/frontend/src/components/VHeader/VHeaderMobile.vue b/frontend/src/components/VHeader/VHeaderMobile.vue new file mode 100644 index 00000000000..154c11bf069 --- /dev/null +++ b/frontend/src/components/VHeader/VHeaderMobile.vue @@ -0,0 +1,405 @@ + + + diff --git a/frontend/src/components/VMediaInfo/VRelatedMedia.vue b/frontend/src/components/VMediaInfo/VRelatedMedia.vue index 80253c682ee..1286c458716 100644 --- a/frontend/src/components/VMediaInfo/VRelatedMedia.vue +++ b/frontend/src/components/VMediaInfo/VRelatedMedia.vue @@ -33,7 +33,7 @@ watch( { immediate: true } ) -const isFetching = computed(() => relatedMediaStore.fetchState.isFetching) +const isFetching = computed(() => relatedMediaStore.fetchState.status === 'loading') const showRelated = computed( () => results.value.items.length > 0 || isFetching.value ) diff --git a/frontend/src/composables/use-collection.ts b/frontend/src/composables/use-collection.ts index ffee63b4e59..3d0e2b69876 100644 --- a/frontend/src/composables/use-collection.ts +++ b/frontend/src/composables/use-collection.ts @@ -18,7 +18,7 @@ export const useCollection = ({ const searchStore = useSearchStore() const collectionParams = computed(() => searchStore.collectionParams) - const isFetching = computed(() => mediaStore.fetchState.isFetching) + const isFetching = computed(() => mediaStore.fetchState.status === 'loading') const media = ref(mediaStore.resultItems[mediaType]) as Ref const creatorUrl = ref() diff --git a/frontend/src/composables/use-search.ts b/frontend/src/composables/use-search.ts index b13bd7af1b3..66d854cb143 100644 --- a/frontend/src/composables/use-search.ts +++ b/frontend/src/composables/use-search.ts @@ -73,7 +73,7 @@ export const useSearch = ( return navigateTo(searchPath) } - const isFetching = computed(() => mediaStore.fetchState.isFetching) + const isFetching = computed(() => mediaStore.fetchState.status === 'loading') const resultsCount = computed(() => mediaStore.resultCount) const { getI18nCount, getLoading } = useI18nResultsCount() diff --git a/frontend/src/middleware/search.ts b/frontend/src/middleware/search.ts index 182196ce3d5..a1814361b6c 100644 --- a/frontend/src/middleware/search.ts +++ b/frontend/src/middleware/search.ts @@ -49,7 +49,7 @@ export const searchMiddleware = defineNuxtRouteMiddleware(async (to) => { const mediaStore = useMediaStore() const results = await mediaStore.fetchMedia() - const fetchingError = mediaStore.fetchState.fetchingError + const fetchingError = mediaStore.fetchState.status === 'error' ? mediaStore.fetchState.error : null if (!results.length && fetchingError && !handledClientSide(fetchingError)) { showError(createError(fetchingError)) } diff --git a/frontend/src/pages/search.vue b/frontend/src/pages/search.vue index 2065df09393..d61add90687 100644 --- a/frontend/src/pages/search.vue +++ b/frontend/src/pages/search.vue @@ -100,8 +100,8 @@ const fetchMedia = async (payload: { shouldPersistMedia?: boolean } = {}) => { return fetchingError.value } -const fetchingError = computed(() => fetchState.value.fetchingError) -const isFetching = computed(() => fetchState.value.isFetching) +const fetchingError = computed(() => fetchState.value.status === 'error' ? fetchState.value.error : null) +const isFetching = computed(() => fetchState.value.status === 'loading') /** * This watcher fires even when the queries are equal. We update the path only diff --git a/frontend/src/stores/media/related-media.ts b/frontend/src/stores/media/related-media.ts index 5d07d5a9ee6..c1ad1e7fa92 100644 --- a/frontend/src/stores/media/related-media.ts +++ b/frontend/src/stores/media/related-media.ts @@ -16,7 +16,7 @@ interface RelatedMediaState { export const useRelatedMediaStore = defineStore("related-media", { state: (): RelatedMediaState => ({ mainMediaId: null, - fetchState: { isFetching: false, hasStarted: false, fetchingError: null }, + fetchState: { status: 'idle', error: null }, media: [], }), @@ -25,23 +25,22 @@ export const useRelatedMediaStore = defineStore("related-media", { (state) => (id: string): Media | undefined => state.media.find((item) => item.id === id), + isFetching: (state) => state.fetchState.status === 'loading', }, actions: { _endFetching(error?: FetchingError) { - this.fetchState.fetchingError = error || null - this.fetchState.hasStarted = true - this.fetchState.isFetching = false + if (error) { + this.fetchState = { status: 'error', error } + } else { + this.fetchState = { status: 'success', error: null } + } }, _startFetching() { - this.fetchState.isFetching = true - this.fetchState.hasStarted = true - this.fetchState.fetchingError = null + this.fetchState = { status: 'loading', error: null } }, _resetFetching() { - this.fetchState.isFetching = false - this.fetchState.hasStarted = false - this.fetchState.fetchingError = null + this.fetchState = { status: 'idle', error: null } }, async fetchMedia(mediaType: SupportedMediaType, id: string) { diff --git a/frontend/src/stores/media/single-result.ts b/frontend/src/stores/media/single-result.ts index 52184de3658..62db292c9a4 100644 --- a/frontend/src/stores/media/single-result.ts +++ b/frontend/src/stores/media/single-result.ts @@ -26,7 +26,7 @@ export const useSingleResultStore = defineStore("single-result", { mediaType: null, mediaId: null, mediaItem: null, - fetchState: { isFetching: false, fetchingError: null }, + fetchState: { status: 'idle', error: null }, }), getters: { @@ -42,16 +42,19 @@ export const useSingleResultStore = defineStore("single-result", { } return null }, + isFetching: (state) => state.fetchState.status === 'loading', }, actions: { _endFetching(error?: FetchingError) { - this.fetchState.isFetching = false - this.fetchState.fetchingError = error || null + if (error) { + this.fetchState = { status: 'error', error } + } else { + this.fetchState = { status: 'success', error: null } + } }, _startFetching() { - this.fetchState.isFetching = true - this.fetchState.fetchingError = null + this.fetchState = { status: 'loading', error: null } }, _updateFetchState(action: "start" | "end", option?: FetchingError) { @@ -66,8 +69,7 @@ export const useSingleResultStore = defineStore("single-result", { this.mediaItem = null this.mediaType = null this.mediaId = null - this.fetchState.isFetching = false - this.fetchState.fetchingError = null + this.fetchState = { status: 'idle', error: null } }, /** diff --git a/frontend/src/stores/provider.ts b/frontend/src/stores/provider.ts index e3bd51b5ab6..ca23eb9534a 100644 --- a/frontend/src/stores/provider.ts +++ b/frontend/src/stores/provider.ts @@ -46,7 +46,7 @@ export const useProviderStore = defineStore("provider", { [AUDIO]: [], [IMAGE]: [], }, - fetchState: { isFetching: false, fetchingError: null }, + fetchState: { status: 'idle', error: null }, sourceNames: { [AUDIO]: [], [IMAGE]: [], @@ -55,13 +55,14 @@ export const useProviderStore = defineStore("provider", { actions: { _endFetching(error?: FetchingError) { - this.fetchState.fetchingError = error || null if (error) { - this.fetchState.isFinished = true + this.fetchState = { status: 'error', error } + } else { + this.fetchState = { status: 'success', error: null } } }, _startFetching() { - this.fetchState.isFetching = true + this.fetchState = { status: 'loading', error: null } }, _updateFetchState(action: "start" | "end", option?: FetchingError) { diff --git a/frontend/test/unit/specs/stores/media-store-fetching.spec.js b/frontend/test/unit/specs/stores/media-store-fetching.spec.js index dbac93d800a..ec5b7cf03b9 100644 --- a/frontend/test/unit/specs/stores/media-store-fetching.spec.js +++ b/frontend/test/unit/specs/stores/media-store-fetching.spec.js @@ -1,5 +1,3 @@ -// @vitest-environment jsdom - import { AxiosError } from "axios" import { createPinia, setActivePinia } from "~~/test/unit/test-utils/pinia" @@ -213,10 +211,8 @@ describe("fetchMedia", () => { }) expect(mediaStore.fetchState).toEqual({ - fetchingError: null, - hasStarted: true, - isFetching: false, - isFinished: false, + status: 'success', + error: null, }) }) }) diff --git a/frontend/test/unit/specs/stores/media-store.spec.ts b/frontend/test/unit/specs/stores/media-store.spec.ts index 9c5402bf2c8..15e63fab077 100644 --- a/frontend/test/unit/specs/stores/media-store.spec.ts +++ b/frontend/test/unit/specs/stores/media-store.spec.ts @@ -80,16 +80,12 @@ describe("media store", () => { }) expect(mediaStore.mediaFetchState).toEqual({ audio: { - fetchingError: null, - hasStarted: false, - isFetching: false, - isFinished: false, + error: null, + status: 'idle', }, image: { - fetchingError: null, - hasStarted: false, - isFetching: false, - isFinished: false, + error: null, + status: 'idle', }, }) }) @@ -188,36 +184,28 @@ describe("media store", () => { it.each` searchType | audioError | fetchState ${ALL_MEDIA} | ${{ code: NO_RESULT }} | ${{ - fetchingError: null, - hasStarted: true, - isFetching: true, - isFinished: false, + error: null, + status: 'loading', }} ${ALL_MEDIA} | ${{ statusCode: 429 }} | ${{ - fetchingError: { + error: { requestKind: "search", statusCode: 429, searchType: ALL_MEDIA, }, - hasStarted: true, - isFetching: true, - isFinished: false, + status: 'loading', }} ${AUDIO} | ${{ statusCode: 429 }} | ${{ - fetchingError: { + error: { requestKind: "search", statusCode: 429, searchType: AUDIO, }, - hasStarted: true, - isFetching: false, - isFinished: true, + status: 'error', }} ${IMAGE} | ${null} | ${{ - fetchingError: null, - hasStarted: true, - isFetching: true, - isFinished: false, + error: null, + status: 'loading', }} `( "fetchState for $searchType returns $fetchState", @@ -251,14 +239,12 @@ describe("media store", () => { }) expect(mediaStore.fetchState).toEqual({ - fetchingError: { + error: { requestKind: "search", code: NO_RESULT, searchType: ALL_MEDIA, }, - hasStarted: true, - isFetching: false, - isFinished: true, + status: 'error', }) }) @@ -270,10 +256,8 @@ describe("media store", () => { mediaStore._updateFetchState(IMAGE, "end") expect(mediaStore.fetchState).toEqual({ - fetchingError: null, - hasStarted: true, - isFetching: false, - isFinished: false, + error: null, + status: 'success', }) }) @@ -299,16 +283,14 @@ describe("media store", () => { }) expect(mediaStore.fetchState).toEqual({ - fetchingError: { + error: { code: "NO_RESULT", message: "Error", requestKind: "search", searchType: ALL_MEDIA, statusCode: 500, }, - hasStarted: true, - isFetching: false, - isFinished: true, + status: 'error', }) }) }) diff --git a/frontend/test/unit/specs/stores/single-result-store.spec.ts b/frontend/test/unit/specs/stores/single-result-store.spec.ts index 8cce22e5f5f..3ed7e7e027b 100644 --- a/frontend/test/unit/specs/stores/single-result-store.spec.ts +++ b/frontend/test/unit/specs/stores/single-result-store.spec.ts @@ -1,5 +1,3 @@ -// @vitest-environment jsdom - import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" import { createPinia, setActivePinia } from "~~/test/unit/test-utils/pinia" import { getAudioObj } from "~~/test/unit/fixtures/audio" @@ -79,8 +77,8 @@ describe("Media Item Store", () => { describe("state", () => { it("sets default state", () => { expect(singleResultStore.fetchState).toEqual({ - isFetching: false, - fetchingError: null, + status: 'idle', + error: null, }) expect(singleResultStore.mediaItem).toEqual(null) expect(singleResultStore.mediaType).toEqual(null) @@ -152,4 +150,4 @@ describe("Media Item Store", () => { expect(singleResultStore.mediaId).toEqual(mediaItem.id) }) }) -}) +}