fix(typing): add undefined to FetchData Promise return type

[csorfab]

The fetchData function of useClientRequest returns an empty Promise if the hook is unmounted. Typing has been updated to reflect this behavior

What does this PR do?

It fixes a typing error that affects useClientRequest's fetchData function, and as a result, useManualQuery and useMutation too. useClientRequest's fetchData function returns an empty (undefined) Promise if called after the component has unmounted, but this is not currently reflected in its typing. Admittedly, this is an edge case behavior, but it's caused my team hard to reproduce bugs, and since the behavior is codified in useClientRequest.test.js:476 ("returns undefined instantly if not mounted"), it seems appropriate that the typing reflect it.

This would be a breaking change for many projects, but I believe that Typescript's usefulness lies exactly in preventing hard-to-reproduce bugs like those caused by this behavior, so I think it's a small price to pay for correctness.

Related issues

Closes #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: c9f9647

[simoneb]

can you please create an issue and use this PR to propose a solution for it?

[csorfab]

Hi @simoneb, I've created issue #772 and linked to this PR in it.

[csorfab]

Another solution would be to return a { loading: false, cacheHit: false } object instead of the empty Promise.resolve() in useClientRequest - this would not be a breaking change for Typescript users, and I don't imagine many depends on undefined being returned in this edge case. I got discouraged from this route since the tests explicitly test for behavior, but I think it might be a better solution than what the current PR achieves? @simoneb what do you think about this? I'd gladly make a PR for that too

[simoneb]

Can you please cover this change with a test by changing or adding to the code in https://github.com/nearform/graphql-hooks/tree/master/packages/graphql-hooks/test-d?

[simoneb]

@csorfab ping :)

[csorfab]

@simoneb sorry for the delay! I checked the typing tests with npm run tsd and it ran fine, so I added two assertions that cover the change in the PR.

Have you considered the other option, which would be returning a "dummy" object ({ loading: false, cacheHit: false }) instead of undefined? I'm beginning to see it as a better option, because it feels like more consistent behavior - maybe an Error could be included, describing that the request was aborted because the hook unmounted? Certainly feels better than just returning undefined without any explanation

[tiagoheliob]

@csorfab Could you please draft an implementation using your suggestion of returning a dummy object? it's certainly more suitable than an undefined return for sure as you mentioned

[csorfab]

@tiagoheliob Hi! I've created a new PR with my implementation of the suggestion: #777

[tiagoheliob]

@csorfab Thanks a lot, I'll take a look

[csorfab]

I'm closing this as #777 has been merged instead