From c69bc3982834c997cbb441e8d36d823827b84fe9 Mon Sep 17 00:00:00 2001 From: NielsJPeschel Date: Thu, 9 Jan 2025 13:38:25 -0500 Subject: [PATCH 1/5] fix: correctly check for promise in useLoadData --- hooks/useLoadData/useLoadData.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/hooks/useLoadData/useLoadData.ts b/hooks/useLoadData/useLoadData.ts index b6c6e85..bd31b50 100644 --- a/hooks/useLoadData/useLoadData.ts +++ b/hooks/useLoadData/useLoadData.ts @@ -1,5 +1,5 @@ import {useEffect, useState, useMemo} from 'react'; -import {ApiResponse, RetryResponse, ApiResponseBase, OptionalDependency, DependencyBase} from '../../types'; +import {ApiResponse, RetryResponse, ApiResponseBase, OptionalDependency, DependencyBase, Promisable} from '../../types'; import {FetchData, NotUndefined} from './types'; @@ -30,6 +30,17 @@ function unboxApiResponse(arg: ApiResponse | T): T { } } +/* + isPromise determines a promise by checking whether or not it is an instanceof + native promise (preferred) or whether it has a then method. +*/ +function isPromise(promisable: Promisable): promisable is Promise { + return ( + promisable instanceof Promise || + (promisable && typeof promisable === 'object' && 'then' in promisable && typeof promisable.then === 'function') + ); +} + export interface LoadDataConfig { fetchWhenDepsChange?: boolean; maxRetryCount?: number; @@ -186,7 +197,8 @@ export function useLoadData( } }, [counter, localFetchWhenDepsChange]); - const nonPromiseResult = initialPromise.res instanceof Promise ? undefined : initialPromise.res; + const initialPromiseRes = initialPromise.res; + const nonPromiseResult = isPromise(initialPromiseRes) ? undefined : initialPromiseRes; const initialData = data || nonPromiseResult; // Initialize our pending data to one of three possible states: From 321caefc25fe6c00693cd397cac4ae2c6bcac1f9 Mon Sep 17 00:00:00 2001 From: NielsJPeschel Date: Thu, 9 Jan 2025 13:46:04 -0500 Subject: [PATCH 2/5] simplification --- hooks/useLoadData/useLoadData.ts | 5 +---- package.json | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/hooks/useLoadData/useLoadData.ts b/hooks/useLoadData/useLoadData.ts index bd31b50..bb7216a 100644 --- a/hooks/useLoadData/useLoadData.ts +++ b/hooks/useLoadData/useLoadData.ts @@ -35,10 +35,7 @@ function unboxApiResponse(arg: ApiResponse | T): T { native promise (preferred) or whether it has a then method. */ function isPromise(promisable: Promisable): promisable is Promise { - return ( - promisable instanceof Promise || - (promisable && typeof promisable === 'object' && 'then' in promisable && typeof promisable.then === 'function') - ); + return promisable && typeof promisable === 'object' && 'then' in promisable && typeof promisable.then === 'function'; } export interface LoadDataConfig { diff --git a/package.json b/package.json index 99385ef..e60a986 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@optum/react-hooks", - "version": "1.0.4", + "version": "1.0.5-next.1", "description": "A reusable set of React hooks", "repository": "https://github.com/Optum/react-hooks", "license": "Apache 2.0", From 01d1dc46888562b86bf20b2d5b7e9371a6760a1a Mon Sep 17 00:00:00 2001 From: NielsJPeschel Date: Fri, 10 Jan 2025 12:48:38 -0500 Subject: [PATCH 3/5] simplification --- hooks/useLoadData/useLoadData.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/hooks/useLoadData/useLoadData.ts b/hooks/useLoadData/useLoadData.ts index bb7216a..6d5b7f3 100644 --- a/hooks/useLoadData/useLoadData.ts +++ b/hooks/useLoadData/useLoadData.ts @@ -30,11 +30,11 @@ function unboxApiResponse(arg: ApiResponse | T): T { } } -/* - isPromise determines a promise by checking whether or not it is an instanceof - native promise (preferred) or whether it has a then method. -*/ function isPromise(promisable: Promisable): promisable is Promise { + /* + simply checking promisable instanceof Promise is not sufficient. + Certain environments to not use native promises + */ return promisable && typeof promisable === 'object' && 'then' in promisable && typeof promisable.then === 'function'; } @@ -194,8 +194,7 @@ export function useLoadData( } }, [counter, localFetchWhenDepsChange]); - const initialPromiseRes = initialPromise.res; - const nonPromiseResult = isPromise(initialPromiseRes) ? undefined : initialPromiseRes; + const nonPromiseResult = isPromise(initialPromise.res) ? undefined : initialPromise.res; const initialData = data || nonPromiseResult; // Initialize our pending data to one of three possible states: From 6aaa5bd590365d50a9083b4723bcd62ca6f334ec Mon Sep 17 00:00:00 2001 From: NielsJPeschel Date: Fri, 10 Jan 2025 14:45:20 -0500 Subject: [PATCH 4/5] add unit test to ensure useLoadData handles thenable objects as promises --- hooks/useLoadData/useLoadData.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hooks/useLoadData/useLoadData.test.ts b/hooks/useLoadData/useLoadData.test.ts index 53977f6..5a3f17a 100644 --- a/hooks/useLoadData/useLoadData.test.ts +++ b/hooks/useLoadData/useLoadData.test.ts @@ -442,4 +442,13 @@ describe('useLoadData', () => { expect(getSuccess).toHaveBeenCalledTimes(2); expect(getSuccess).toHaveBeenCalledWith('b'); }); + + it('should treat a thenable object as a Promise', async () => { + const getThenableSuccess = jest.fn(() => ({then: (resolve: any) => resolve(successResult)})); + + const {result} = renderHook(() => useLoadData(getThenableSuccess)); + expect(result.current.isInProgress).toBe(true); + await waitFor(() => expect(result.current.isInProgress).toBe(false)); + expect(result.current.result).toBe(successResult); + }); }); From 2262827d5c8c2720998f1032327d0fa45472aaa1 Mon Sep 17 00:00:00 2001 From: NielsJPeschel Date: Fri, 10 Jan 2025 14:47:47 -0500 Subject: [PATCH 5/5] improved comment --- hooks/useLoadData/useLoadData.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hooks/useLoadData/useLoadData.ts b/hooks/useLoadData/useLoadData.ts index 6d5b7f3..bf4eb9e 100644 --- a/hooks/useLoadData/useLoadData.ts +++ b/hooks/useLoadData/useLoadData.ts @@ -32,8 +32,9 @@ function unboxApiResponse(arg: ApiResponse | T): T { function isPromise(promisable: Promisable): promisable is Promise { /* - simply checking promisable instanceof Promise is not sufficient. - Certain environments to not use native promises + Simply checking promisable instanceof Promise is not sufficient. + Certain environments do not use native promises. Checking for promisable + to be thenable is a more comprehensive and conclusive test. */ return promisable && typeof promisable === 'object' && 'then' in promisable && typeof promisable.then === 'function'; }