diff --git a/client/web-sveltekit/prettier.config.cjs b/client/web-sveltekit/prettier.config.cjs index 018e218ead9..2dd0d464cce 100644 --- a/client/web-sveltekit/prettier.config.cjs +++ b/client/web-sveltekit/prettier.config.cjs @@ -2,5 +2,8 @@ const baseConfig = require('../../prettier.config.js') module.exports = { ...baseConfig, plugins: [...(baseConfig.plugins || []), 'prettier-plugin-svelte'], - overrides: [...(baseConfig.overrides || []), { files: '*.svelte', options: { parser: 'svelte', htmlWhitespaceSensitivity: 'strict' } }], + overrides: [ + ...(baseConfig.overrides || []), + { files: '*.svelte', options: { parser: 'svelte', htmlWhitespaceSensitivity: 'strict' } }, + ], } diff --git a/client/web-sveltekit/src/lib/graphql/urql.test.ts b/client/web-sveltekit/src/lib/graphql/urql.test.ts index fb9010ec4fd..03abc045397 100644 --- a/client/web-sveltekit/src/lib/graphql/urql.test.ts +++ b/client/web-sveltekit/src/lib/graphql/urql.test.ts @@ -1,215 +1,192 @@ import { type AnyVariables, Client, type OperationResult, CombinedError, cacheExchange } from '@urql/core' -import { test, expect, vi, beforeEach } from 'vitest' +import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest' import { pipe, filter, map, merge } from 'wonka' -import { infinityQuery } from './urql' +import { IncrementalRestoreStrategy, infinityQuery } from './urql' -function getMockClient(responses: Partial>[]): Client { - return new Client({ - url: '#testingonly', - exchanges: [ - cacheExchange, // This is required because infiniteQuery expects that a cache exchange is present - ({ forward }) => - operations$ => { - const mockResults$ = pipe( - operations$, - filter(operation => { - switch (operation.kind) { - case 'query': - case 'mutation': - return true - default: - return false - } - }), - map((operation): OperationResult => { - const response = responses.shift() - if (!response) { +describe('infinityQuery', () => { + function getMockClient(responses: Partial>[]): Client { + return new Client({ + url: '#testingonly', + exchanges: [ + cacheExchange, // This is required because infiniteQuery expects that a cache exchange is present + ({ forward }) => + operations$ => { + const mockResults$ = pipe( + operations$, + filter(operation => { + switch (operation.kind) { + case 'query': + case 'mutation': + return true + default: + return false + } + }), + map((operation): OperationResult => { + const response = responses.shift() + if (!response) { + return { + operation, + error: new CombinedError({ + networkError: new Error('No more responses'), + }), + stale: false, + hasNext: false, + } + } return { + ...response, operation, - error: new CombinedError({ - networkError: new Error('No more responses'), - }), + data: response.data ?? undefined, + error: response.error ?? undefined, stale: false, hasNext: false, } - } - return { - ...response, - operation, - data: response.data ?? undefined, - error: response.error ?? undefined, - stale: false, - hasNext: false, - } - }) - ) + }) + ) - const forward$ = pipe( - operations$, - filter(operation => { - switch (operation.kind) { - case 'query': - case 'mutation': - return false - default: - return true - } - }), - forward - ) + const forward$ = pipe( + operations$, + filter(operation => { + switch (operation.kind) { + case 'query': + case 'mutation': + return false + default: + return true + } + }), + forward + ) - return merge([mockResults$, forward$]) - }, - ], - }) -} + return merge([mockResults$, forward$]) + }, + ], + }) + } -function getQuery(client: Client) { - return infinityQuery({ - client, - query: 'query { list { nodes { id } } pageInfo { hasNextPage, endCursor } } }', - variables: { - first: 2, - afterCursor: null as string | null, - }, - nextVariables: previousResult => { - if (previousResult?.data?.list?.pageInfo?.hasNextPage) { + function getQuery(client: Client) { + return infinityQuery({ + client, + query: 'query { list { nodes { id } } pageInfo { hasNextPage, endCursor } } }', + variables: { + first: 2, + afterCursor: null as string | null, + }, + map: result => { + const list = result.data?.list return { - afterCursor: previousResult.data.list.pageInfo.endCursor, + nextVariables: list?.pageInfo.hasNextPage ? { afterCursor: list.pageInfo.endCursor } : undefined, + data: list?.nodes, + error: result.error, } - } - return undefined - }, - combine: (previousResult, nextResult) => { - if (!nextResult.data?.list) { - return nextResult - } - const previousNodes = previousResult.data?.list?.nodes ?? [] - const nextNodes = nextResult.data.list?.nodes ?? [] - return { - ...nextResult, + }, + merge: (prev, next) => [...(prev ?? []), ...(next ?? [])], + createRestoreStrategy: api => + new IncrementalRestoreStrategy( + api, + n => n.length, + n => ({ first: n.length }) + ), + }) + } + + let query: ReturnType + + beforeEach(() => { + vi.useFakeTimers() + + const client = getMockClient([ + { data: { list: { - ...nextResult.data.list, - nodes: [...previousNodes, ...nextNodes], - }, - }, - } - }, - }) -} - -let query: ReturnType - -beforeEach(() => { - vi.useFakeTimers() - - const client = getMockClient([ - { - data: { - list: { - nodes: [{ id: 1 }, { id: 2 }], - pageInfo: { - hasNextPage: true, - endCursor: '2', + nodes: [{ id: 1 }, { id: 2 }], + pageInfo: { + hasNextPage: true, + endCursor: '2', + }, }, }, }, - }, - { - data: { - list: { - nodes: [{ id: 3 }, { id: 4 }], - pageInfo: { - hasNextPage: true, - endCursor: '4', + { + data: { + list: { + nodes: [{ id: 3 }, { id: 4 }], + pageInfo: { + hasNextPage: true, + endCursor: '4', + }, }, }, }, - }, - { - data: { - list: { - nodes: [{ id: 5 }, { id: 6 }], - pageInfo: { - hasNextPage: false, + { + data: { + list: { + nodes: [{ id: 5 }, { id: 6 }], + pageInfo: { + hasNextPage: false, + }, }, }, }, - }, - ]) - query = getQuery(client) -}) - -test('fetch more', async () => { - const subscribe = vi.fn() - query.subscribe(subscribe) - - await vi.runAllTimersAsync() - - // 1. call: fetching -> true - // 2. call: result - expect(subscribe).toHaveBeenCalledTimes(2) - expect(subscribe.mock.calls[0][0]).toMatchObject({ - fetching: true, + ]) + query = getQuery(client) }) - expect(subscribe.mock.calls[1][0]).toMatchObject({ - fetching: false, - data: { - list: { - nodes: [{ id: 1 }, { id: 2 }], - pageInfo: { - hasNextPage: true, - endCursor: '2', - }, + + afterEach(() => { + vi.useRealTimers() + }) + + test('fetch more', async () => { + const subscribe = vi.fn() + query.subscribe(subscribe) + + await vi.runAllTimersAsync() + + // 1. call: fetching -> true + // 2. call: result + expect(subscribe).toHaveBeenCalledTimes(2) + expect(subscribe.mock.calls[0][0]).toMatchObject({ + fetching: true, + }) + expect(subscribe.mock.calls[1][0]).toMatchObject({ + fetching: false, + data: [{ id: 1 }, { id: 2 }], + nextVariables: { + afterCursor: '2', }, - }, - }) + }) - // Fetch more data - query.fetchMore() - await vi.runAllTimersAsync() + // Fetch more data + query.fetchMore() + await vi.runAllTimersAsync() - // 3. call: fetching -> true - // 4. call: result - expect(subscribe).toHaveBeenCalledTimes(4) - expect(subscribe.mock.calls[2][0]).toMatchObject({ - fetching: true, - }) - expect(subscribe.mock.calls[3][0]).toMatchObject({ - fetching: false, - data: { - list: { - nodes: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }], - pageInfo: { - hasNextPage: true, - endCursor: '4', - }, + // 3. call: fetching -> true + // 4. call: result + expect(subscribe).toHaveBeenCalledTimes(4) + expect(subscribe.mock.calls[2][0]).toMatchObject({ + fetching: true, + }) + expect(subscribe.mock.calls[3][0]).toMatchObject({ + fetching: false, + data: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }], + nextVariables: { + afterCursor: '4', }, - }, - }) -}) - -test('restoring state', async () => { - const subscribe = vi.fn() - query.subscribe(subscribe) - await vi.runAllTimersAsync() - await query.restore(result => (result.data as any).list.nodes.length < 5) - - expect(subscribe).toHaveBeenCalledTimes(6) - expect(subscribe.mock.calls[4][0]).toMatchObject({ - restoring: true, - }) - expect(subscribe.mock.calls[5][0]).toMatchObject({ - restoring: false, - data: { - list: { - nodes: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }, { id: 5 }, { id: 6 }], - pageInfo: { - hasNextPage: false, - }, - }, - }, + }) + }) + + test('restoring state', async () => { + const subscribe = vi.fn() + query.subscribe(subscribe) + const snapshot = query.capture() + query.restore({ ...snapshot!, count: 6 }) + await vi.runAllTimersAsync() + + expect(subscribe.mock.calls[3][0]).toMatchObject({ + fetching: false, + data: [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }, { id: 5 }, { id: 6 }], + }) }) }) diff --git a/client/web-sveltekit/src/lib/graphql/urql.ts b/client/web-sveltekit/src/lib/graphql/urql.ts index a4fb7ecdaf1..9bdddf19f53 100644 --- a/client/web-sveltekit/src/lib/graphql/urql.ts +++ b/client/web-sveltekit/src/lib/graphql/urql.ts @@ -2,22 +2,20 @@ import { Client, cacheExchange, fetchExchange, - mapExchange, - type Exchange, makeOperation, + mapExchange, type AnyVariables, - type OperationResult, - createRequest, type DocumentInput, + type Exchange, + type OperationResult, } from '@urql/core' import type { OperationDefinitionNode } from 'graphql' import { once } from 'lodash' -import { from, isObservable, Subject, type Observable, concat, of } from 'rxjs' -import { map, switchMap, scan, startWith } from 'rxjs/operators' -import { type Readable, readable, get } from 'svelte/store' +import { type Readable, get, writable, type Writable } from 'svelte/store' import type { GraphQLResult } from '@sourcegraph/http-client' +import { uniqueID } from '$lib/dom' import { GRAPHQL_URI } from '$lib/http-client' import { getHeaders } from './shared' @@ -65,7 +63,7 @@ export function query(query, variables).toPromise() } -interface InfinityQueryArgs { +interface InfinityQueryArgs { /** * The {@link Client} instance to use for the query. */ @@ -74,76 +72,112 @@ interface InfinityQueryArgs + query: DocumentInput /** * The initial variables to use for the query. */ - variables: TVariables | Observable + variables: TVariables | Promise /** - * A function that returns the next set of variables to use for the query. + * Process the result of the query. This function maps the response to the data used + * and computes the next set of query variables, if any. * + * @param result - The result of the query. * @param previousResult - The previous result of the query. - * - * @remarks - * `nextVariables` is called when {@link InfinityQueryStore.fetchMore} is called to get the next set - * of variables to fetch the next page of data. This function to extract the cursor for the next - * page from the previous result. + * @returns The new/combined result state. */ - nextVariables: (previousResult: OperationResult) => Partial | undefined + map: (result: OperationResult) => InfinityStoreResult /** - * A function to combine the previous result with the next result. - * - * @param previousResult - The previous result of the query. - * @param nextResult - The next result of the query. - * @returns The combined result of the query. - * - * @remarks - * `combine` is called when the next result is received to merge the previous result with the new - * result. This function is used to append the new data to the previous data. + * Optional callback to merge the data from the previous result with the new data. + * If not provided the new data will replace the old data. */ - combine: ( - previousResult: OperationResultState, - nextResult: OperationResultState - ) => OperationResultState + merge?: (previousData: TData | undefined, newData: TData | undefined) => TData + + /** + * Returns a strategy for restoring the data when navigating back to a page. + */ + createRestoreStrategy?: (api: InfinityAPI) => RestoreStrategy } -interface OperationResultState - extends OperationResult { +/** + * Internal API for the infinity query store. Used by restore strategies to control the store. + */ +interface InfinityAPI { + /** + * The internal store representing the current state of the query. + */ + store: Writable> + /** + * Helper function for fetching and processing the next set of data. + */ + fetch( + variables: Partial, + previous: InfinityStoreResult + ): Promise> +} + +/** + * The processed/combined result of a GraphQL query. + */ +export interface InfinityStoreResult { + data?: TData + + /** + * Set if there was an error fetching the data. When set, no more data will be fetched. + */ + error?: Error + + /** + * The set of variables to use for the next fetch. If not set no more data will be fetched. + */ + nextVariables?: Partial +} + +/** + * The state of the infinity query store. + */ +interface InfinityStoreResultState + extends InfinityStoreResult { /** * Whether a GraphQL request is currently in flight. */ fetching: boolean - /** - * Whether the store is currently restoring data. - */ - restoring: boolean } // This needs to be exported so that TS type inference can work in SvelteKit generated files. -export interface InfinityQueryStore - extends Readable> { +export interface InfinityQueryStore + extends Readable> { /** - * Reruns the query with the next set of variables returned by {@link InfinityQueryArgs.nextVariables}. + * Reruns the query with the next set of query variables. * * @remarks - * A new query will only be executed if there is no query currently in flight and {@link InfinityQueryArgs.nextVariables} - * returns a value different from `undefined`. + * A new query will only be executed if there is no query currently in flight and {@link InfinityStoreResult.nextVariables} + * is set. */ fetchMore: () => void /** - * Fetches more data until the given restoreHandler returns `false`. - * - * @param restoreHandler - A function that returns `true` if more data should be fetched. - * - * @remarks - * When navigating back to a page that was previously fetched with `infinityQuery`, the page - * should call `restore` until the previous data state is restored. + * Fetches data while the given predicate is true. Using this function is different f + * rom calling {@link fetchMore} in a loop, because it will set/unset the fetching state + * only once. */ - restore: (restoreHandler: (result: OperationResultState) => boolean) => Promise + fetchWhile: (predicate: (data: TData) => boolean) => Promise + + /** + * Restores the data state from a snapshot, which is returned by {@link capture}. + * + * @param snapshot - The snapshot to restore. + * @returns A promise that resolves when the data has been restored. + */ + restore: (snapshot: TSnapshot | undefined) => Promise + + /** + * Captures the current data state to a snapshot that can be used to restore the data later. + * @returns The snapshot. + */ + capture: () => TSnapshot | undefined } /** @@ -157,125 +191,216 @@ export interface InfinityQueryStore( - args: InfinityQueryArgs -): InfinityQueryStore { - // This is a hacky workaround to create an initialState. The empty object is - // invalid but the request will never be executed with these variables anyway. - const initialVariables = isObservable(args.variables) ? args.variables : of(args.variables) - const operation = args.client.createRequestOperation( - 'query', - isObservable(args.variables) - ? createRequest(args.query, {} as TVariables) - : createRequest(args.query, args.variables) - ) - const initialState: OperationResultState = { - operation, - error: undefined, - data: undefined, - extensions: undefined, - stale: false, - fetching: false, - restoring: false, - hasNext: false, +export function infinityQuery< + TData = any, + TPayload = any, + TVariables extends AnyVariables = AnyVariables, + TSnapshot = void +>(args: InfinityQueryArgs): InfinityQueryStore { + const initialVariables = Promise.resolve(args.variables) + + async function fetch( + variables: Partial, + previousResult: InfinityStoreResult + ): Promise> { + const result = args.map( + await initialVariables.then(initialVariables => + args.client.query(args.query, { ...initialVariables, ...variables }) + ) + ) + if (args.merge) { + result.data = args.merge(previousResult.data, result.data) + } + return result } - const nextVariables = new Subject>() - let shouldRestore: ((result: OperationResultState) => boolean) | null = null + + const initialState: InfinityStoreResultState = { fetching: true } + const store = writable(initialState) + const restoreStrategy = args.createRestoreStrategy?.({ store, fetch }) // Prefetch data. We don't want to wait until the store is subscribed to. That allows us to use this function // inside a data loader and the data will be prefetched before the component is rendered. - initialVariables.subscribe(variables => { - void args.client.query(args.query, variables).toPromise() + fetch({}, {}).then(result => { + store.update(current => { + // Only set the initial state if we haven't already started another fetch process, + // e.g. when restoring the state. + if (current === initialState) { + return { ...result, fetching: false } + } + return current + }) }) - const result = readable(initialState, set => { - const subscription = initialVariables - .pipe( - switchMap(initialVariables => - nextVariables.pipe( - startWith(initialVariables), // nextVaribles will not emit until the first fetchMore is called - switchMap(variables => { - const operation = args.client.createRequestOperation( - 'query', - createRequest(args.query, { ...initialVariables, ...variables }) - ) - return concat( - of({ fetching: true, stale: false, restoring: false }), - from(args.client.executeRequestOperation(operation).toPromise()).pipe( - map(({ data, stale, operation, error, extensions }) => ({ - fetching: false, - data, - stale: !!stale, - operation, - error, - extensions, - })) - ) - ) - }) - ) - ), - scan((result, update) => { - const newResult = { ...result, ...update } - return update.fetching ? newResult : args.combine(result, newResult) - }, initialState) - ) - .subscribe(result => { - if (shouldRestore) { - result.restoring = Boolean( - (result.data || result.error) && shouldRestore(result) && args.nextVariables(result) - ) + /** + * Resolves when the store is not fetching anymore. + */ + function waitTillReady(): Promise { + let unsubscribe: () => void + return new Promise(resolve => { + unsubscribe = store.subscribe(current => { + if (!current.fetching) { + resolve() } - set(result) }) - - return () => subscription.unsubscribe() - }) + }).finally(() => unsubscribe()) + } return { - ...result, + subscribe: store.subscribe, fetchMore: () => { - const current = get(result) - if (current.fetching || current.restoring) { + const previous = get(store) + + if (previous.fetching) { + // When a fetch is already in progress, we don't want to start another one for the same variables. return } - const newVariables = args.nextVariables(current) - if (!newVariables) { - return - } - nextVariables.next(newVariables) - }, - restore: restoreHandler => { - shouldRestore = result => { - return Boolean((result.data || result.error) && restoreHandler(result) && args.nextVariables(result)) - } - return new Promise(resolve => { - const unsubscribe = result.subscribe(result => { - if (result.fetching) { - return - } - if (result.data || result.error) { - const newVariables = args.nextVariables(result) - if (restoreHandler(result) && newVariables) { - shouldRestore = restoreHandler - nextVariables.next(newVariables) - } else { - unsubscribe() - shouldRestore = null - resolve() + + if (previous.nextVariables && !previous.error) { + store.set({ ...previous, fetching: true }) + fetch(previous.nextVariables, previous).then(result => { + store.update(current => { + if (previous.nextVariables === current.nextVariables) { + return { ...result, fetching: false } } - } + return current + }) }) - }) + } }, + fetchWhile: async predicate => { + // We need to wait until the store is not fetching anymore to ensure that we don't start + // another fetch process while one is already in progress. + await waitTillReady() + const current = get(store) + + store.set({ ...current, fetching: true }) + + let result: InfinityStoreResult = current + while (!result.error && result.nextVariables && result.data && predicate(result.data)) { + result = await fetch(result.nextVariables, result) + } + + store.set({ ...result, fetching: false }) + }, + capture: () => restoreStrategy?.capture(get(store)), + restore: snapshot => { + if (restoreStrategy && snapshot) { + return restoreStrategy.restore(snapshot) + } + return Promise.resolve() + }, + } +} + +/** + * A restore strategy captures and restores the data state of a query. + */ +interface RestoreStrategy { + capture(result: InfinityStoreResult): TSnapshot | undefined + restore(snapshot: TSnapshot): Promise +} + +// This needs to be exported so that TS type inference can work in SvelteKit generated files. +export interface IncrementalRestoreStrategySnapshot { + count: number + variables?: Partial + nonce: string +} + +// We use this to indentify snapshots that were created in the current "session", which +// means there is a high chance that the data is still in the cache. +const NONCE = uniqueID('repeat-restore') + +/** + * The incremental restore strategy captures and restores the data by counting the number of items. + * It will fetch more data until the count matches the snapshot count. + * + * This strategy is useful when every fetch returns a fixed number of items (i.e. after a cursor). + * In this case we want to make use of our GraphQL client's caching strategy and simply + * "replay" the previous fetches. + * + * This strategy works well when GraphQL requests are cached. To avoid waterfall requests in case the + * data is not cached, the strategy will fall back to requesting the data once with query variables + * from the snapshot. + */ +export class IncrementalRestoreStrategy + implements RestoreStrategy, TData> +{ + constructor( + private api: InfinityAPI, + /** + * A function to map the data to a number. This number will be used to count the items. + */ + private mapper: (data: TData) => number, + /** + * A function to map the data to query variables. These variables will be used to fetch the data + * once when if there is a chance that the data is not in the cache (fallback). + */ + private variablesMapper?: (data: TData) => Partial + ) {} + + public capture(result: InfinityStoreResult): IncrementalRestoreStrategySnapshot | undefined { + return result.data + ? { + count: this.mapper(result.data), + variables: this.variablesMapper ? this.variablesMapper(result.data) : undefined, + nonce: NONCE, + } + : undefined + } + + public async restore(snapshot: IncrementalRestoreStrategySnapshot): Promise { + this.api.store.set({ fetching: true }) + const result = await (snapshot.nonce !== NONCE && snapshot.variables + ? this.api.fetch(snapshot.variables, {}) + : this.fetch(snapshot)) + this.api.store.set({ ...result, fetching: false }) + } + + private async fetch( + snapshot: IncrementalRestoreStrategySnapshot + ): Promise> { + let current: InfinityStoreResult = { nextVariables: {} } + while (current.nextVariables && ((current.data && this.mapper(current.data)) || 0) < snapshot.count) { + current = await this.api.fetch(current.nextVariables, current) + if (current.error || !current.data) { + break + } + } + return current + } +} + +/** + * A restore strategy that overwrites the current store state with the response of a new query. + * The strategy uses the query variables form the snapshot to fetch the data. + */ +export class OverwriteRestoreStrategy + implements RestoreStrategy<{ variables: Partial }, TData> +{ + constructor( + private api: InfinityAPI, + private variablesMapper: (data: TData) => Partial + ) {} + + capture(result: InfinityStoreResult): { variables: Partial } | undefined { + if (!result.data) { + return undefined + } + const variables = this.variablesMapper(result.data) + return variables ? { variables } : undefined + } + + async restore(snapshot: { variables: Partial }): Promise { + this.api.store.set({ fetching: true }) + const result = await this.api.fetch(snapshot.variables, {}) + this.api.store.set({ ...result, fetching: false }) } } diff --git a/client/web-sveltekit/src/lib/repo/HistoryPanel.stories.svelte b/client/web-sveltekit/src/lib/repo/HistoryPanel.stories.svelte index 67e0326d704..948bbefe2ff 100644 --- a/client/web-sveltekit/src/lib/repo/HistoryPanel.stories.svelte +++ b/client/web-sveltekit/src/lib/repo/HistoryPanel.stories.svelte @@ -2,12 +2,15 @@ import { createHistoryResults } from '$testing/testdata' import { Story } from '@storybook/addon-svelte-csf' import HistoryPanel from './HistoryPanel.svelte' + import { readable } from 'svelte/store' export const meta = { component: HistoryPanel, parameters: { sveltekit_experimental: { stores: { - page: {}, + page: { + url: new URL(window.location.href), + }, }, }, }, @@ -17,12 +20,19 @@

Commits to show:


{#key commitCount} - {}} /> + {/key}
diff --git a/client/web-sveltekit/src/lib/repo/HistoryPanel.svelte b/client/web-sveltekit/src/lib/repo/HistoryPanel.svelte index 6a2f1c82e8f..b34d9cd88dc 100644 --- a/client/web-sveltekit/src/lib/repo/HistoryPanel.svelte +++ b/client/web-sveltekit/src/lib/repo/HistoryPanel.svelte @@ -1,76 +1,64 @@ - - {#if history} + + {#if $history.data} - {#each history.nodes as commit (commit.id)} + {#each $history.data as commit (commit.id)} {@const selected = commit.abbreviatedOID === selectedRev || commit.oid === selectedRev}
@@ -111,12 +99,22 @@ {/each}
{/if} - {#if !history || loading} - + {#if $history.fetching} +
+ +
+ {:else if $history.error} +
+ Unable to load history: {$history.error.message} +
{/if}