From 68055135bc27008a1eebef514544fa3b6c9305fa Mon Sep 17 00:00:00 2001 From: Dorian Maliszewski Date: Mon, 24 Jan 2022 10:10:41 +0100 Subject: [PATCH] feat: improve dataloader refetch handling --- .../use-dataloader/src/DataLoaderProvider.tsx | 1 - .../src/__tests__/DataLoaderProvider.test.tsx | 52 ++++----- .../src/__tests__/dataloader.test.ts | 110 +++++++----------- packages/use-dataloader/src/dataloader.ts | 47 +++----- packages/use-dataloader/src/types.ts | 1 + packages/use-dataloader/src/useDataLoader.ts | 12 +- 6 files changed, 96 insertions(+), 127 deletions(-) diff --git a/packages/use-dataloader/src/DataLoaderProvider.tsx b/packages/use-dataloader/src/DataLoaderProvider.tsx index fdc429c93..1d8c35b16 100644 --- a/packages/use-dataloader/src/DataLoaderProvider.tsx +++ b/packages/use-dataloader/src/DataLoaderProvider.tsx @@ -20,7 +20,6 @@ type CachedData = Record type Reloads = Record Promise> type UseDataLoaderInitializerArgs = { - enabled?: boolean status?: StatusEnum method: () => PromiseType pollingInterval?: number diff --git a/packages/use-dataloader/src/__tests__/DataLoaderProvider.test.tsx b/packages/use-dataloader/src/__tests__/DataLoaderProvider.test.tsx index 8c1f8244a..42b0b8c99 100644 --- a/packages/use-dataloader/src/__tests__/DataLoaderProvider.test.tsx +++ b/packages/use-dataloader/src/__tests__/DataLoaderProvider.test.tsx @@ -54,11 +54,11 @@ describe('DataLoaderProvider', () => { expect(Object.values(result.current.getReloads()).length).toBe(1) const testRequest = result.current.getRequest(TEST_KEY) expect(testRequest).toBeDefined() - expect(testRequest?.status).toBe(StatusEnum.IDLE) + expect(testRequest.status).toBe(StatusEnum.IDLE) act(() => { // launch should never throw // eslint-disable-next-line @typescript-eslint/no-floating-promises - testRequest?.launch() + testRequest.load() }) expect(method).toBeCalledTimes(1) expect(testRequest.status).toBe(StatusEnum.LOADING) @@ -92,11 +92,11 @@ describe('DataLoaderProvider', () => { const testRequest = result.current.getRequest(TEST_KEY) expect(Object.values(result.current.getReloads()).length).toBe(1) expect(testRequest).toBeDefined() - expect(testRequest?.status).toBe(StatusEnum.IDLE) + expect(testRequest.status).toBe(StatusEnum.IDLE) act(() => { // launch should never throw // eslint-disable-next-line @typescript-eslint/no-floating-promises - testRequest?.launch() + testRequest.load() }) expect(method).toBeCalledTimes(1) await waitFor(() => expect(testRequest.getData()).toBe(true)) @@ -112,14 +112,14 @@ describe('DataLoaderProvider', () => { wrapper, }) act(() => { - result.current.getOrAddRequest(TEST_KEY, { - enabled: true, + const request = result.current.getOrAddRequest(TEST_KEY, { method, }) result.current.getOrAddRequest(TEST_KEY, { - enabled: true, method, }) + // eslint-disable-next-line @typescript-eslint/no-floating-promises + request.load() }) expect(method).toBeCalledTimes(1) await waitFor(() => @@ -198,25 +198,25 @@ describe('DataLoaderProvider', () => { wrapper: wrapperWith2ConcurrentRequests, }) act(() => { - result.current.addRequest(TEST_KEY, { - enabled: true, - method, - }) - result.current.addRequest(`${TEST_KEY}-2`, { - enabled: true, - method, - }) - result.current.addRequest(`${TEST_KEY}-3`, { - enabled: true, - method, - }) - result.current.addRequest(`${TEST_KEY}-4`, { - enabled: true, - method, - }) - result.current.addRequest(`${TEST_KEY}-5`, { - enabled: true, - method, + ;[ + result.current.addRequest(TEST_KEY, { + method, + }), + result.current.addRequest(`${TEST_KEY}-2`, { + method, + }), + result.current.addRequest(`${TEST_KEY}-3`, { + method, + }), + result.current.addRequest(`${TEST_KEY}-4`, { + method, + }), + result.current.addRequest(`${TEST_KEY}-5`, { + method, + }), + ].forEach(request => { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + request.load() }) }) expect(method).toBeCalledTimes(2) diff --git a/packages/use-dataloader/src/__tests__/dataloader.test.ts b/packages/use-dataloader/src/__tests__/dataloader.test.ts index 2b43bc88d..0adc4a7f9 100644 --- a/packages/use-dataloader/src/__tests__/dataloader.test.ts +++ b/packages/use-dataloader/src/__tests__/dataloader.test.ts @@ -24,7 +24,7 @@ const fakeErrorPromise = () => }) describe('Dataloader class', () => { - test('should create instance not enabled then load then destroy', async () => { + test('should create instance then load then destroy', async () => { const method = jest.fn(fakeSuccessPromise) const instance = new DataLoader({ key: 'test', @@ -33,53 +33,44 @@ describe('Dataloader class', () => { expect(instance.status).toBe(StatusEnum.IDLE) expect(method).toBeCalledTimes(0) await instance.load() + expect(instance.status).toBe(StatusEnum.SUCCESS) expect(method).toBeCalledTimes(1) await instance.destroy() instance.clearData() }) - test('should create instance enabled with cancel', async () => { + test('should create instance with cancel', async () => { const notify = jest.fn() const method = jest.fn(fakeSuccessPromise) const instance = new DataLoader({ - enabled: true, key: 'test', method, }) instance.addObserver(notify) - expect(instance.status).toBe(StatusEnum.LOADING) expect(instance.getData()).toBe(undefined) expect(notify).toBeCalledTimes(0) + // eslint-disable-next-line @typescript-eslint/no-floating-promises + instance.load() expect(method).toBeCalledTimes(1) await instance.cancel() - await new Promise(resolve => { - setTimeout(() => { - expect(notify).toBeCalledTimes(1) - resolve(true) - }, PROMISE_TIMEOUT) - }) + expect(notify).toBeCalledTimes(1) expect(instance.getData()).toBe(undefined) instance.clearData() }) - test('should create instance enabled without cancel', async () => { + test('should create instance without cancel', async () => { const notify = jest.fn() const method = jest.fn(fakeSuccessPromise) const instance = new DataLoader({ - enabled: true, key: 'test', method, }) instance.addObserver(notify) - expect(instance.status).toBe(StatusEnum.LOADING) expect(notify).toBeCalledTimes(0) + expect(instance.status).toBe(StatusEnum.IDLE) + await instance.load() + expect(notify).toBeCalledTimes(2) expect(method).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(() => { - expect(notify).toBeCalledTimes(1) - resolve(true) - }, PROMISE_TIMEOUT) - }) expect(instance.getData()).toBe(true) instance.getObserversCount() instance.removeObserver(notify) @@ -87,36 +78,26 @@ describe('Dataloader class', () => { instance.clearData() }) - test('should create instance enabled with null data', async () => { + test('should create instance with null data', async () => { const method = jest.fn(fakeNullPromise) const instance = new DataLoader({ - enabled: true, key: 'test', method, }) - expect(instance.status).toBe(StatusEnum.LOADING) + expect(instance.status).toBe(StatusEnum.IDLE) + await instance.load() expect(method).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(() => { - resolve(true) - }, PROMISE_TIMEOUT) - }) expect(instance.getData()).toBe(undefined) }) - test('should create instance enabled with undefined data', async () => { + test('should create instance with undefined data', async () => { const method = jest.fn(fakeUndefinedPromise) const instance = new DataLoader({ - enabled: true, key: 'test', method, }) - expect(instance.status).toBe(StatusEnum.LOADING) + expect(instance.status).toBe(StatusEnum.IDLE) + await instance.load() expect(method).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(() => { - resolve(true) - }, PROMISE_TIMEOUT) - }) expect(instance.getData()).toBe(undefined) }) @@ -129,8 +110,8 @@ describe('Dataloader class', () => { }) instance.addOnCancelListener(onCancel) instance.addOnCancelListener(onCancel) - // eslint-disable-next-line no-void - void instance.load() + // eslint-disable-next-line @typescript-eslint/no-floating-promises + instance.load() await instance.cancel() expect(onCancel).toBeCalledTimes(1) instance.removeOnCancelListener(onCancel) @@ -146,8 +127,8 @@ describe('Dataloader class', () => { }) instance.addOnCancelListener(onCancel) instance.addOnCancelListener(onCancel) - // eslint-disable-next-line no-void - void instance.load() + // eslint-disable-next-line @typescript-eslint/no-floating-promises + instance.load() await instance.cancel() expect(onCancel).toBeCalledTimes(1) instance.removeOnCancelListener(onCancel) @@ -190,30 +171,31 @@ describe('Dataloader class', () => { const instance = new DataLoader({ key: 'test', method, - pollingInterval: PROMISE_TIMEOUT * 2, + pollingInterval: PROMISE_TIMEOUT, }) await instance.load() expect(method).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(resolve, PROMISE_TIMEOUT * 3) + await waitForExpect(() => { + expect(method).toBeCalledTimes(2) }) - expect(method).toBeCalledTimes(2) - await new Promise(resolve => { - setTimeout(resolve, PROMISE_TIMEOUT * 3) + await waitForExpect(() => { + expect(method).toBeCalledTimes(3) }) - expect(method).toBeCalledTimes(3) await instance.load() await instance.load() await waitForExpect(() => { expect(method).toBeCalledTimes(4) }) + await waitForExpect(() => { + expect(method).toBeCalledTimes(5) + }) await instance.load() await instance.load() await instance.load(true) await waitForExpect(() => { expect(method).toBeCalledTimes(6) }) - instance.setPollingInterval(PROMISE_TIMEOUT * 4) + instance.setPollingInterval(PROMISE_TIMEOUT * 2) await instance.destroy() }) @@ -227,16 +209,14 @@ describe('Dataloader class', () => { }) await instance.load() expect(method).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(resolve, PROMISE_TIMEOUT * 3) + // eslint-disable-next-line @typescript-eslint/no-floating-promises + await instance.load() + await waitForExpect(() => { + expect(method).toBeCalledTimes(2) }) - expect(method).toBeCalledTimes(2) - await new Promise(resolve => { - setTimeout(resolve, PROMISE_TIMEOUT * 3) + await waitForExpect(() => { + expect(method).toBeCalledTimes(3) }) - expect(method).toBeCalledTimes(3) - await instance.load() - await instance.load() await waitForExpect(() => { expect(method).toBeCalledTimes(4) }) @@ -244,7 +224,7 @@ describe('Dataloader class', () => { await instance.load() await instance.load(true) await waitForExpect(() => { - expect(method).toBeCalledTimes(6) + expect(method).toBeCalledTimes(5) }) instance.setPollingInterval(PROMISE_TIMEOUT * 4) await instance.destroy() @@ -272,24 +252,20 @@ describe('Dataloader class', () => { const method = jest.fn(fakeSuccessPromise) const onSuccess = jest.fn() const instance = new DataLoader({ - enabled: true, key: 'test', - maxDataLifetime: PROMISE_TIMEOUT, + maxDataLifetime: PROMISE_TIMEOUT * 3, method, }) instance.addOnSuccessListener(onSuccess) - expect(instance.status).toBe(StatusEnum.LOADING) + expect(instance.status).toBe(StatusEnum.IDLE) + await instance.load() expect(method).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(resolve, PROMISE_TIMEOUT) - }) expect(onSuccess).toBeCalledTimes(1) await instance.load() expect(method).toBeCalledTimes(1) expect(onSuccess).toBeCalledTimes(1) - await new Promise(resolve => { - setTimeout(resolve, PROMISE_TIMEOUT * 2) - }) + // Wait until data is outdated + await waitForExpect(() => expect(instance.isDataOutdated).toBeTruthy()) await instance.load() expect(method).toBeCalledTimes(2) expect(onSuccess).toBeCalledTimes(2) @@ -300,11 +276,11 @@ describe('Dataloader class', () => { DataLoader.maxConcurrent = 2 for (let i = 0; i < 5; i += 1) { const instance = new DataLoader({ - enabled: true, key: `test-${i}`, method, }) - expect(instance.status).toBe(StatusEnum.LOADING) + // eslint-disable-next-line @typescript-eslint/no-floating-promises + instance.load() } // Because wait for setTimeout tryLaunch in dataloader.ts await waitForExpect(() => { diff --git a/packages/use-dataloader/src/dataloader.ts b/packages/use-dataloader/src/dataloader.ts index bb96d24d4..2116d86df 100644 --- a/packages/use-dataloader/src/dataloader.ts +++ b/packages/use-dataloader/src/dataloader.ts @@ -8,7 +8,6 @@ import { } from './types' export type DataLoaderConstructorArgs = { - enabled?: boolean key: string method: () => PromiseType pollingInterval?: number @@ -26,8 +25,6 @@ class DataLoader { public key: string - private enabled: boolean - public status: StatusEnum public pollingInterval?: number @@ -64,28 +61,13 @@ class DataLoader { public constructor(args: DataLoaderConstructorArgs) { this.key = args.key - this.enabled = args.enabled ?? false - this.status = args.enabled ? StatusEnum.LOADING : StatusEnum.IDLE + this.status = StatusEnum.IDLE this.method = args.method this.pollingInterval = args?.pollingInterval this.keepPreviousData = args?.keepPreviousData this.maxDataLifetime = args.maxDataLifetime this.needPolling = args.needPolling ?? true - if (args.enabled) { - this.tryLaunch() - } else { - this.notifyChanges() - } - } - - private tryLaunch(): void { - if (DataLoader.started < DataLoader.maxConcurrent) { - // Because we want to launch the request directly without waiting the return - // eslint-disable-next-line no-void - void this.load() - } else { - setTimeout(() => this.tryLaunch()) - } + this.notifyChanges() } public getData(): T | undefined { @@ -99,18 +81,26 @@ class DataLoader { public load = async (force = false): Promise => { if ( force || - this.status !== StatusEnum.SUCCESS || + this.status === StatusEnum.IDLE || + this.status === StatusEnum.ERROR || (this.status === StatusEnum.SUCCESS && this.isDataOutdated) ) { - if (this.timeout) { - // Prevent multiple call at the same time - clearTimeout(this.timeout) + if (DataLoader.started < DataLoader.maxConcurrent) { + // Because we want to launch the request directly without waiting the return + if (this.timeout) { + // Prevent multiple call at the same time + clearTimeout(this.timeout) + } + await this.launch() + } else { + await new Promise(resolve => { + setTimeout(resolve) + }).then(() => this.load(force)) } - await this.launch() } } - public launch = async (): Promise => { + private launch = async (): Promise => { try { if (this.status !== StatusEnum.LOADING) { this.canceled = false @@ -142,6 +132,8 @@ class DataLoader { this.isDataOutdated = true this.notifyChanges() }, this.maxDataLifetime) as unknown as number + } else { + this.isDataOutdated = true } } this.notifyChanges() @@ -247,9 +239,6 @@ class DataLoader { public setPollingInterval(newPollingInterval?: number): void { this.pollingInterval = newPollingInterval - if (this.enabled) { - this.tryLaunch() - } } public setNeedPolling(needPolling: NeedPollingType): void { diff --git a/packages/use-dataloader/src/types.ts b/packages/use-dataloader/src/types.ts index 514b1aaa2..fb36463d8 100644 --- a/packages/use-dataloader/src/types.ts +++ b/packages/use-dataloader/src/types.ts @@ -53,6 +53,7 @@ export interface UseDataLoaderResult { isLoading: boolean isPolling: boolean isSuccess: boolean + isFirstLoad: boolean previousData?: T reload: () => Promise } diff --git a/packages/use-dataloader/src/useDataLoader.ts b/packages/use-dataloader/src/useDataLoader.ts index 94fd743b8..bf459ab76 100644 --- a/packages/use-dataloader/src/useDataLoader.ts +++ b/packages/use-dataloader/src/useDataLoader.ts @@ -47,7 +47,6 @@ const useDataLoader = ( } const newRequest = getOrAddRequest(fetchKey, { - enabled, keepPreviousData, maxDataLifetime, method, @@ -60,7 +59,6 @@ const useDataLoader = ( return newRequest }, [ - enabled, fetchKey, getOrAddRequest, maxDataLifetime, @@ -107,11 +105,16 @@ const useDataLoader = ( [isSuccess, isLoading, enabled, pollingInterval], ) + const isFirstLoad = useMemo( + () => (enabled && isIdle) || isLoading, + [isLoading, isIdle, enabled], + ) + useEffect(() => { - if (enabled && isIdle) { + if (enabled) { // launch should never throw // eslint-disable-next-line @typescript-eslint/no-floating-promises - request.launch?.() + request.load() } }, [request, enabled, isIdle]) @@ -157,6 +160,7 @@ const useDataLoader = ( data: request.getData() || (initialData as T), error: request?.error, isError, + isFirstLoad, isIdle, isLoading, isPolling,