fix(defaultFetcher): Avoid known hang in node-fetch

code-forger commented 3 years ago

There is a known hang in node-fetch. We avoid it by avoiding the clone function.

Description

As per https://github.com/node-fetch/node-fetch#custom-highwatermark there is a known hang when consuming the body of a request with a response that is larger than the default buffer size.

In this PR I avoid this hang by removing the call to .clone(). Instead we always read the body as text, and then try and parse it ourselves.

Motivation and Context

This change prevents instances where requests in nodeJS (such as during SSR uses of fetchye) would indefinitely hang.

How Has This Been Tested?

Using fetchye as part of a One App module, I have created SSR requests that tests the correct behaviour for both json payloads and non-json payloads. I would have liked to have added a unit test that would have failed for the old code, however since we completely mock out node-fetch in the tests, there is no way to reproduce this hang. Ive modified the unit tests to ensure that the old behaviour has been maintained. This required more changes to tests than I would have liked, however since the 'happy' case now relies on the body of the request being parsable JSON these changes were needed.

Types of Changes

[x] 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.
[x] My changes are in sync with the code style of this project.
[x] 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?

There should be no impact for developers as there should be no change to the api.

CLAassistant commented 3 years ago

All committers have signed the CLA.

codesandbox-ci[bot] commented 3 years ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit be7627dcb9e19c1b0220cc508653800ece396e53:

Sandbox	Source
quick-install	Configuration
fetchye-provider-install	Configuration
fetchye-redux-provider-install	Configuration
nextjs-fetchye-ssr	Configuration

americanexpress / fetchye