Fix/is loading/multi render behaviour weirdness #99

DyBev · 2025-07-04T08:36:26Z

Description

There is a small edge case where when a call is waiting for another call to finish - think A2 needs data from A1.
on the first render
A2 options.defer = true; isLoading == false as a result;
A1 options.defer = false && data = undefined; isLoading == true as a result;

After A1 data has been fetched, A2 options.defer = false.
However because it isn't the first render isLoading == false, and the loading state is only set to true during the runAsync function call.

To remedy this I've added an extra check that is the same as the check in the useEffect which will be checking (!options.defer && !data && !isLoading && !error) if all of these statements are true then we can assume that the runAsync function will be triggered in the useEffect.

Motivation and Context

potentially solves issue #81
Also potentially solves an issue found by a team in American express.

How Has This Been Tested?

Manual testing was completed following the reproduction steps as outlined on issue #81, as well as manual testing on the branch where the issue was found.

Currently available fetchye package. repository

Fetchye package using a build produced from this branch. repository

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)
Dependency update

Checklist:

My change requires a change to the documentation and I have updated the documentation accordingly.
My changes are in sync with the code style of this project.
There aren't any other open Pull Requests for the same issue/update.
These changes should be applied to a maintenance branch.
I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using Fetchye?

DyBev · 2025-07-04T08:45:47Z

With this change could the first render logic be removed from the isLoading function?

DyBev · 2025-07-09T10:15:41Z

Added the code changes in #98 to a branch here and ran the package locally.

the changes don't interfer with my changes. Will need to re-create the implementation of #98 to test if my changes interfer with that

DyBev · 2025-07-23T08:17:23Z

Converting to draft whilst pr #102 is open

DyBev · 2025-08-20T10:01:42Z

Testing for sequential calls

Fetchye v1.7.1

Fetchye with these changes

DyBev · 2025-08-20T12:32:34Z

Testing for id changing after render

note: there is potentially an issue where the initial return will return the data from the previous id. see #105

Fetchye with these changes

DyBev · 2025-08-20T12:39:07Z

testing again forceFetch functionality

Fetchye 1.7.1

Fetchye with these changes

code-forger

Thanks for the contribution, I think there is one logic regression. if you could make some unit tests to prove this is a regression, then fix it, that would be great.

I've also asked for a slight reinforcing of the unit tests, possibly even in some that you did not author. Since you are changing rather complex logic, laying down more tests will help the package consumers be confident that this change has no logic regressions.

code-forger · 2025-09-16T09:15:46Z

packages/fetchye/src/queryHelpers.js

+  // if defer is true
+  if (options.defer) {
+    // isLoading should be false
+    return false;


If the caller passes in three quick renders:

defer: true

defer: false

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) {

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.

code-forger · 2025-09-16T09:20:34Z

packages/fetchye/__tests__/queryHelpers.spec.js

    expect(isLoading({
-      loading: false, data: undefined, numOfRenders: 1, options: { forceInitialFetch: true },
+      loading: false,
+      numOfRenders: 1,


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

Removed in this commit e45edff

code-forger · 2025-09-16T09:24:02Z

packages/fetchye/__tests__/queryHelpers.spec.js

+  it('should return false defer is true', () => {
+    expect(isLoading({
+      loading: false, data: undefined, options: { defer: true }, error: undefined, refs: {},
+    })).toEqual(false);


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

(same for other tests in this file)

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

code-forger · 2025-09-16T09:24:18Z

packages/fetchye/__tests__/queryHelpers.spec.js

 } from '../src/queryHelpers';

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


Suggested change

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

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

added in this commit e45edff

DyBev added 2 commits July 4, 2025 09:24

fix(isLoading): added catch for post render edge case

b29ae5f

test(isLoading): added tests

b00d90d

github-actions bot added the one-app-team-review-requested label Jul 4, 2025

chore(clean-up): removed numOfRenders from isLoading function

6e80500

DyBev marked this pull request as ready for review July 4, 2025 10:20

DyBev requested review from a team as code owners July 4, 2025 10:20

chore(sync): added code to sync changes with pr#98

fd44eb6

DyBev force-pushed the fix/isLoading/multi-render-behaviour-weirdness branch from 2b7e0ba to fd44eb6 Compare July 9, 2025 10:05

Matthew-Mallimo force-pushed the main branch from 33881e2 to e8e91d2 Compare July 15, 2025 17:43

DyBev marked this pull request as draft July 23, 2025 08:17

chore(merge): 'main'

bbfb724

DyBev force-pushed the fix/isLoading/multi-render-behaviour-weirdness branch from 6dd2d78 to bbfb724 Compare August 11, 2025 17:12

DyBev mentioned this pull request Aug 20, 2025

isLoading flag is false for a pending request that follows cancelled request #106

Open

DyBev added 2 commits August 20, 2025 10:57

chore(merge): 'main'

15be263

feat(forceInitialFetch): added forceInitialFetch functionality

286db6c

Matthew-Mallimo previously approved these changes Sep 10, 2025

View reviewed changes

DyBev marked this pull request as ready for review September 11, 2025 07:45

code-forger requested changes Sep 16, 2025

View reviewed changes

DyBev added 2 commits September 23, 2025 20:03

refactor(pr-comments): acconting for regression

c75cceb

test(queryHelpers): added extra tests and edge cases

e45edff

DyBev dismissed Matthew-Mallimo’s stale review via e45edff September 23, 2025 19:05

chore(clean-up): updated tests to be grouped together

cb0ffc1

DyBev requested a review from Matthew-Mallimo September 24, 2025 15:49

DyBev requested a review from code-forger September 24, 2025 15:49

Matthew-Mallimo enabled auto-merge (squash) November 13, 2025 14:13

Matthew-Mallimo approved these changes Nov 13, 2025

View reviewed changes

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

Fix/is loading/multi render behaviour weirdness #99

Are you sure you want to change the base?

Fix/is loading/multi render behaviour weirdness #99

Uh oh!

Conversation

DyBev commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

What is the Impact to Developers Using Fetchye?

Uh oh!

DyBev commented Jul 4, 2025

Uh oh!

DyBev commented Jul 9, 2025

Uh oh!

DyBev commented Jul 23, 2025

Uh oh!

DyBev commented Aug 20, 2025

Testing for sequential calls

Uh oh!

DyBev commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing for id changing after render

Uh oh!

DyBev commented Aug 20, 2025

testing again forceFetch functionality

Uh oh!

code-forger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DyBev Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DyBev commented Jul 4, 2025 •

edited

Loading

DyBev commented Aug 20, 2025 •

edited

Loading

DyBev Sep 24, 2025 •

edited

Loading