feat(fetchData): no undefined when unmounted

[csorfab]

useClientRequests's fetchData no longer returns undefined when the hook is unmounted,
instead returns a result object with a descriptive error

What does this PR do?

It resolves #772 by returning a result object conforming to the typing and normal behavior of useClientRequest's fetchData

Related issues

#772

Checklist

• I have checked the contributing document
• I have added or updated any relevant documentation
• I have added or updated any relevant tests

[netlify]

👷 Deploy request for graphql-hooks-cra pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: f2d1947

[tiagoheliob]

@csorfab just let a comment about returning Promise.reject instead of Promise.resolve

[simoneb]

@csorfab thanks for all the work you've done here. My main concern with this change is that it's a breaking change and because I don't know the motivations that led to the current implementation I wouldn't feel comfortable changing it right now.

If you agree with these considerations, can we go back to your first PR and only get the typing change in?

[csorfab]

@simoneb Well, the thing is, the other PR would be a major breaking change for Typescript users, for example it would be no longer possible to destructure the result of an awaited fetchData because you can't destructure undefined, and people would have to spam their code with type assertions or optional chaining. This PR is in line with the typing and the way graphql-hooks is intended to be used, and while I obviously don't know the original intent behind the empty resolve(), it definitely feels more like a quick hack, than intended behavior. Calling fetchData after it unmounted is an edge case, and I have a hard time imagining that anyone's code would depend on fetchData returning undefined in that very specific edge case - you always need to check for result.error under normal circumstances, and the original behavior causes bugs with that, that's what happened with me and what motivated this PR. If anything, I think this would fix unnoticed bugs in people's code rather than break anything.

All that said, I absolutely understand your concerns, this is definitely not an easy call

[csorfab]

@simoneb I dug up the PR that originally introduced this behavior: #96 (related issue: #54)

It seems to me that no particular thought was given to what should be returned in this case, it was just a quick bug fix

[tiagoheliob]

@csorfab #96 seems like a quick fix like you mentioned, what Simone meant is that people are currently doing something like the example below would have these changes to break their code

current

const  something = await fetchData(...)

if (!something) { 
 // do something 
 return;
}
// do something else

with the introduced changes

const  something = await fetchData(...)

if (something.error...) { 
 // do something 
 return;
}
// do something else

[csorfab]

@tiagoheliob I understand, but are you sure that people actually do this? Because this behavior is not mentioned anywhere in the docs, nor the typings, and in 99.99% of cases the if(!something) banch will never get called, so it's impossible to find out about it during normal usage (where you follow the something.error/something.data pattern based on the docs).

The only way to find out about this behavior is by looking at the source code after unexpected, hard to reproduce errors (which is how I came about it)

[tiagoheliob]

@csorfab good point, I've made some tests and it's working fine, I'll approve it

[tiagoheliob]

LGTM

[csorfab]

Thank you all for the cooperation!

[simoneb]

Thank you @csorfab for your work and the thought you put into this! This change is now released in the latest version of the package.