Skip to content
38 changes: 29 additions & 9 deletions packages/fetchye/__tests__/queryHelpers.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,49 @@ import {
} from '../src/queryHelpers';

describe('isLoading', () => {
it('should return false defer is true', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should return false defer is true', () => {
it('should return false if defer is true', () => {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in this commit e45edff

expect(isLoading({
loading: false, data: undefined, options: { defer: true }, error: undefined, refs: {},
})).toEqual(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see some more expects per test:

This tests proves that for the above set of inputs, you get false, but does not prove that should return false if defer is true, since you have not checked other combinations of inputs.

So in this same it block, having many combinations of isLoading, data, options, error, and refs, showing that for any combination, when defer: true => false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same for other tests in this file)

Copy link
Author

@DyBev DyBev Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored the tests a little to make use of more describe blocks: cb0ffc1

I've added a should return false and should return true describe blocks with each case in it() this way any failures will highlight the case where it failed, I put them all into 2 "true" or "false" with multiple expects, but when it failed it was tricky to figure out which one was the failing case

});

it('should return true if loading cache true', () => {
expect(isLoading({
loading: true, data: undefined, numOfRenders: 2, options: {},
loading: true, data: undefined, options: {}, error: undefined, refs: {},
})).toEqual(true);
});
it('should return true if first render is true', () => {

it('should return true if all args are false', () => {
expect(isLoading({
loading: false, data: undefined, numOfRenders: 1, options: { },
loading: false, options: { defer: false }, error: undefined, refs: {},
})).toEqual(true);
});
it('should return true if first render and forceInitialFetch option is true', () => {

it('should return true if refs.forceInitialFetch === true', () => {
expect(isLoading({
loading: false, data: undefined, numOfRenders: 1, options: { forceInitialFetch: true },
loading: false,
numOfRenders: 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your function no-longer receives this parameter, please remove it from the test

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in this commit e45edff

data: {},
options: {
defer: false,
forceInitialFetch: true,
},
error: undefined,
refs: {
forceInitialFetch: true,
},
})).toEqual(true);
});
it('should return false if first render is true and defer is true', () => {

it('should return false if there are errors present', () => {
expect(isLoading({
loading: false, data: undefined, numOfRenders: 1, options: { defer: true },
loading: false, options: { defer: false }, error: { }, refs: {},
})).toEqual(false);
});
it('should return false if all args are false', () => {

it('should return false if there is data present', () => {
expect(isLoading({
loading: false, numOfRenders: 2, options: { defer: false },
loading: false, data: { }, numOfRenders: 2, options: { defer: false }, refs: {},
})).toEqual(false);
});
});
Expand Down
35 changes: 16 additions & 19 deletions packages/fetchye/src/queryHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,31 +15,28 @@
*/

export const isLoading = ({
loading, data, numOfRenders, options,
loading, data, options, error, refs,
}) => {
// If first render
if (numOfRenders === 1) {
// and defer is true
if (options.defer) {
// isLoading should be false
return false;
}
// and no data exists and presume loading state isn't present
if (!data) {
// isLoading should be true
return true;
}
// when we force fetch isLoading is always going to be true on first render
if (options.forceInitialFetch) {
// isLoading should be true
return true;
}
// if defer is true
if (options.defer) {
// isLoading should be false
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the caller passes in three quick renders:

  1. defer: true
  2. defer: false
  3. defer: true

then isLoading will now go false despite the fact there may still be a call in flight.

This check might need to be

if (options.defer && numOfRenders === 1) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code c75cceb

I've moved the defer to only stop the function from returning true if fetchye will fetch, rather than an early false return if defer is enabled.

}
// If not on first render and loading from cache is true

// If loading from cache is true
if (loading) {
// isLoading should be true
return true;
}

// Here to mimic what happens in the useEffect
if (
(!data && !error)
|| (refs.forceInitialFetch === true)
) {
return true;
}

// else isLoading should be false
return false;
};
Expand Down
5 changes: 4 additions & 1 deletion packages/fetchye/src/useFetchye.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,11 @@ const useFetchye = (
isLoading: isLoading({
loading: selectorState.current.loading,
data: selectorState.current.data || options.initialData?.data,
numOfRenders: numOfRenders.current,
options,
error: selectorState.current.error,
refs: {
forceInitialFetch: forceInitialFetch.current,
},
}),
error: passInitialData(
{
Expand Down
Loading