Use node-fetch v3 beta and require Node.js 10.16

[xxczaki]

Fixes #8 (allows using the highWaterMark option)

---

We have a section in the node-fetch's readme about custom highWaterMark. Let me know if we should link it somewhere here 😄

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #8: clone() hangs with large response in Node

---

[xxczaki]

Tests are failing on Node.js v8, as it is not supported in the v3 version of node-fetch.

[sholladay]

Thanks for the PR!

I am 👍 on dropping Node 8.

[xxczaki]

@sholladay I added information about how to deal with highWaterMark to FAQ.

[sholladay]

Could you double check the unit measurements? I would expect these values to be measured in multiples of bytes, not bits.

[xxczaki]

Oh, you are right. Just changed it. Thanks for catching that!

[xxczaki]

The default highWaterMark is now 10MB. Should I mention it in the docs or not?

Also, if you want a test for the default highWaterMark, I can add something like this:

test('default highWaterMark is 10MB', async t => {
	const response = await ky('https://httpbin.org/json');
	t.is(response.highWaterMark, 10485760);
});

[sindresorhus]

Should I mention it in the docs or not?

Yes

[xxczaki]

Everything done 😄

[xxczaki]

I updated the node-fetch version so that we can now use the spread syntax. However, it will not work the way you wanted to:

// Changes the default 16384 bytes but does not allow users to specify the `highWaterMark`
fetch(options.url, {...options, highWaterMark: TEN_MEGABYTES});

That's why I used the following approach:

const DEFAULT = 16384;
fetch(options.url, {...options, highWaterMark: options.highWaterMark === DEFAULT ? TEN_MEGABYTES : options.highWaterMark});

Let me know if there is a cleaner way to do it.

[sindresorhus]

How about:

fetch(options.url, {...options, ...{highWaterMark: TEN_MEGABYTES}});

?

[sholladay]

I changed it to::

global.fetch = (url, options) => fetch(url, {highWaterMark: TEN_MEGABYTES, ...options});

Could you give this a try and let me know if it works? If not, I'll need to dig into it further, as the older commit with DEFAULT doesn't really make sense to me.

[xxczaki]

@sindresorhus This changes the default node-fetch's highWaterMark to 10 MB, but does not allow users to override it (so the same behavior as {...options, highWaterMark: TEN_MEGABYTES}).

@sholladay This doesn't change the default node-fetch's highWaterMark (16384 bytes) but does allow users to override it.

Also, we can't do (url, options) => ..., as the options will always return undefined.

Here is the set of tests I use to check the behavior:

test('default highWaterMark is 10MB', async t => {
	const response = await ky('https://httpbin.org/json');
	t.is(response.highWaterMark, 10000000);
});

test('can override the highWaterMark', async t => {
	const response = await ky('https://httpbin.org/json', {highWaterMark: 1024 * 1024});
	t.is(response.highWaterMark, 1048576);
});

In terms of the old solution (with DEFAULT) - it basically sets the highWaterMark to 10MB if options.highWaterMark equals 16384 bytes (node-fetch's default) and otherwise sets it to options.highWaterMark (it was overwritten by the user).

I hope that makes sense.

[xxczaki]

In conclusion, fetch(options.url, {highWaterMark: TEN_MEGABYTES, ...options}) doesn't work, because we are setting the highWaterMark to 10MB, while at the same time ignoring the user preference (options.highWaterMark).

[sholladay]

How is response.highWaterMark being defined in your tests? It's a request option, I'm not sure how it ends up on the response.

@sholladay This doesn't change the default node-fetch's highWaterMark (16384 bytes) but does allow users to override it.

I don't think we care about node-fetch's default, we just want to have our own default, which is what my code does. I could totally be misunderstanding something, just not seeing it yet. Under what circumstances would our default not be applied, other than the user overriding it? And how/why?

Also, we can't do (url, options) => ..., as the options will always return undefined.

This makes me think there might be a misunderstanding in how object spread works. Here's an example with mock fetch functions that take the same arguments and spread the options the same way, which do produce the output I would expect...

const fetchA = (url, options) => {
    fetchB(url, {highWaterMark: 'fetchA default', ...options});
};
const fetchB = (url, options) => {
    console.log('url:', url);
    console.log('options:', options);
};

fetchA();
// url: undefined
// options: { highWaterMark: "fetchA default" }
fetchA('foo');
// url: "foo"
// options: { highWaterMark: "fetchA default" }
fetchA('foo', {});
// url: "foo"
// options: { highWaterMark: "fetchA default" }
fetchA('foo', {highWaterMark: 'custom value'})
// url: "foo"
// options: { highWaterMark: "custom value" }

In other words, it's fine to spread undefined into an object. The object will still be created, the default will still apply, and from fetchB's perspective, there will always be an options object with a highWaterMark, with either the fetchA default, or the user's custom value. In either case, any default inside of fetchB should be ignored.

In this case, fetchA represents ky-universal and fetchB represents node-fetch.

Could you point me more specifically to where you think this logic is wrong?

[xxczaki]

Ok, it turns out I did not understand some things correctly 😆 After doing more verification, I can confirm that the correct highWaterMark is applied in both cases.

[sholladay]

Great, no worries! This is looking good to me now. 👍

My final thought on this is that, ultimately, the node-fetch bug is still there, the stream could still hang if the response is large enough. Looking at their updated docs, I see that they recommend resolving both the original and cloned response at the same time and seem to imply that is the true, proper solution. I'm not sure if we can do that in all cases in Ky, but I think it should be possible in at least one or two of the cases where we clone the response. So that's something to look into for the future.