Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.json() shorthand doesn't resolve in nodejs env #46

Open
iscekic opened this issue Jan 30, 2023 · 5 comments
Open

.json() shorthand doesn't resolve in nodejs env #46

iscekic opened this issue Jan 30, 2023 · 5 comments

Comments

@iscekic
Copy link

iscekic commented Jan 30, 2023

Broken:

import ky from "ky-universal";

const result = await ky
  .post(process.env.BE_ENDPOINT!, {
    json: jsonBody,
  })
  .json();

// we never reach here, await never resolves

Works:

import ky from "ky-universal";

const response = await ky
  .post(process.env.BE_ENDPOINT!, {
    json: jsonBody,
  });

const result = await response.json(); // result is here, yay

deps:

"node": "16.17.1"

"ky": "0.33.2",
"ky-universal": "0.11.0",
"web-streams-polyfill": "3.2.1"

ky-universal#node-fetch@3.3.0

NOTE - Seems to depend on the response body somehow (size?), as it doesn't happen for all requests, but it is consistent within the context of a single request (ie. a broken request/response pair stays broken, a valid request/response pair works).

@iscekic iscekic changed the title .json() shorthand doesn't resolve in a nodejs env .json() shorthand doesn't resolve in nodejs env Jan 30, 2023
@iscekic
Copy link
Author

iscekic commented Jan 31, 2023

The afterResponse hook does fire correctly - here's the logged Response object:

[1] Response {
[1]   size: 0,
[1]   timeout: 0,
[1]   [Symbol(Body internals)]: {
[1]     body: PassThrough {
[1]       _readableState: [ReadableState],
[1]       _events: [Object: null prototype],
[1]       _eventsCount: 5,
[1]       _maxListeners: undefined,
[1]       _writableState: [WritableState],
[1]       allowHalfOpen: true,
[1]       [Symbol(kCapture)]: false,
[1]       [Symbol(kCallback)]: null
[1]     },
[1]     disturbed: false,
[1]     error: null
[1]   },
[1]   [Symbol(Response internals)]: {
[1]     url: 'REDACTED',
[1]     status: 200,
[1]     statusText: 'OK',
[1]     headers: Headers { [Symbol(map)]: [Object: null prototype] },
[1]     counter: undefined
[1]   }
[1] }

@sindresorhus
Copy link
Owner

This is most likely in issue with node-fetch. Ky doesn't have any web reported issues about .json().

@iscekic
Copy link
Author

iscekic commented Jan 31, 2023

Looks like a very similar issue was already reported here: node-fetch/node-fetch#1131

.clone() seems to be the problem.

@jimmywarting
Copy link

jimmywarting commented Jan 31, 2023

I found this: https://github.com/sindresorhus/ky/blob/ae2fe071296702381d30790a19e9137dd51babaa/source/core/Ky.ts#L86-L105

				const awaitedResult = await result;
				const response = awaitedResult.clone();

				if (type === 'json') {
					if (response.status === 204) {
						return '';
					}

					const arrayBuffer = await response.clone().arrayBuffer();
					const responseSize = arrayBuffer.byteLength;
					if (responseSize === 0) {
						return '';
					}

					if (options.parseJson) {
						return options.parseJson(await response.text());
					}
				}

				return response[type]();

to summarize: you clone the response but only consume one of the responses.
in an ideal world you would consume both tee:ed streams at the same time in parallel if you .clone() them

@smeijer
Copy link

smeijer commented Jul 4, 2023

That's it! I've been neck deep into #8, as all symptoms are the same when using the native node (v18.16) fetch, but simply handling the response, releases the process.

It's the afterResponse hook that made my process hang, even when it's a simple noop.

ky({
  // …
  hooks: {
    afterResponse: [
      (request, options, response) => {
        // consume the response stream so node doesn't keep hanging
        response.text().catch(Object);
      },
    ],
  },
});

Is this something that can be fixed in https://github.com/sindresorhus/ky?


It looks like it could. I've patched my local instance, and this seems to work. Are there any downsides of doing so?

for (const hook of ky._options.hooks.afterResponse) {
+   const clone = response.clone();
	// eslint-disable-next-line no-await-in-loop
	const modifiedResponse = await hook(
		ky.request,
		ky._options as NormalizedOptions,
-		ky._decorateResponse(response.clone()),
+		ky._decorateResponse(clone),
	);
+   if (!clone.bodyUsed) clone.text().catch(Object);
	if (modifiedResponse instanceof globalThis.Response) {
		response = modifiedResponse;
	}
}

https://github.com/sindresorhus/ky/blob/356d61c1534c9dc2cfb8ce1c1ff5c4832579e11b/source/core/Ky.ts#L34-L45


And the more simpler fix is to return the response from the hook 😬 . Maybe it's just a matter of updating docs & typs?

ky({
  // …
  hooks: {
    afterResponse: [
      (request, options, response) => {
        // return response so node doesn't keep hanging
        return response;
      },
    ],
  },
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants