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

Add onUploadProgress option #632

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
80 changes: 80 additions & 0 deletions source/core/Ky.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,24 @@
// The spread of `this.request` is required as otherwise it misses the `duplex` option for some reason and throws.
this.request = new globalThis.Request(new globalThis.Request(url, {...this.request}), this._options as RequestInit);
}

// Add onUploadProgress handling
if (this._options.onUploadProgress && typeof this._options.onUploadProgress === 'function') {
if (!supportsRequestStreams) {
throw new Error('Streams are not supported in your environment. `ReadableStream` is missing.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should instead say something about request.duplex not being supported.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy of the message in onDownloadProgress. Should I change that as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is a difference in what causes an environment to lack support for request streams vs response streams. See the implementation of the support constants. In particular, for an environment to support request streams, it needs to have a Request option named duplex, which we have feature detection for. That option is not relevant to response streams, which are much more widely supported.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 'Request streams are not supported in your environment. The duplex option for Request is not available.'. Would this error message be enough?

}

const originalBody = this.request.body;
if (originalBody) {
const totalBytes = this._getTotalBytes(originalBody);
this.request = new Request(this.request, {
body: this._wrapBodyWithUploadProgress(originalBody, totalBytes, this._options.onUploadProgress),
headers: this.request.headers,
method: this.request.method,
signal: this.request.signal,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these properties should be copied over from the previous request automatically and shouldn't need to be set manually like this. Is there a reason this was done?

Copy link
Author

@jadedevin13 jadedevin13 Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason. Changed it to

				this.request
					= new globalThis.Request(this._input, {
						...this._options,
						body: this._streamRequest(
							originalBody, totalBytes, this._options.onUploadProgress),
					});

});
}
}
}

protected _calculateRetryDelay(error: unknown) {
Expand Down Expand Up @@ -365,4 +383,66 @@
},
);
}

protected _getTotalBytes(body: globalThis.BodyInit): number {
if (body instanceof globalThis.Blob) {
return body.size;
}
if (body instanceof globalThis.ArrayBuffer) {

Check failure on line 391 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
return body.byteLength;
}
if (typeof body === 'string') {

Check failure on line 394 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
return new globalThis.TextEncoder().encode(body).length;
}
if (body instanceof URLSearchParams) {

Check failure on line 397 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
return new globalThis.TextEncoder().encode(body.toString()).length;
}
if (body instanceof globalThis.FormData) {

Check failure on line 400 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
// This is an approximation, as FormData size calculation is not straightforward
return Array.from(body.entries()).reduce((acc, [_, value]) => {

Check failure on line 402 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Prefer the spread operator over `Array.from(…)`.

Check failure on line 402 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

`Array#reduce()` is not allowed

Check failure on line 402 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

The variable `acc` should be named `accumulator`. A more descriptive name will do too.
if (typeof value === 'string') {
return acc + new globalThis.TextEncoder().encode(value).length;
}
if (value instanceof Blob) {

Check failure on line 406 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
return acc + value.size;
}
return acc;

Check failure on line 409 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
}, 0);
}
if ('byteLength' in body) {

Check failure on line 412 in source/core/Ky.ts

View workflow job for this annotation

GitHub Actions / Node.js 18

Expected blank line before this statement.
return (body as globalThis.ArrayBufferView).byteLength;
}
return 0; // Default case, unable to determine size
}

protected _wrapBodyWithUploadProgress(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make more sense for this function to have a similar signature to _stream(), i.e. take two arguments, a request and onUploadProgress, and return a request.

We could rename these functions _streamRequest() and _streamResponse().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to _streamRequest and _streamResponse

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge it @sholladay?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had sufficient time to do a proper review or to try it out. And I probably won't for about a week or so. Maybe @sindresorhus or @szmarczak can take a look, otherwise I'll get to it as soon as I can.

body: BodyInit,
totalBytes: number,
onUploadProgress: (progress: { percent: number; transferredBytes: number; totalBytes: number }) => void
): globalThis.ReadableStream<Uint8Array> {
let transferredBytes = 0;

return new globalThis.ReadableStream({
async start(controller) {
const reader = body instanceof globalThis.ReadableStream ? body.getReader() : new globalThis.Response(body).body!.getReader();

async function read() {
const { done, value } = await reader.read();
if (done) {
controller.close();
return;
}

transferredBytes += value.byteLength;
const percent = totalBytes === 0 ? 0 : transferredBytes / totalBytes;
onUploadProgress({ percent, transferredBytes, totalBytes });

controller.enqueue(value);
await read();
}

await read();
},
});
}
}
1 change: 1 addition & 0 deletions source/core/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export const kyOptionKeys: KyOptionsRegistry = {
hooks: true,
throwHttpErrors: true,
onDownloadProgress: true,
onUploadProgress: true,
fetch: true,
};

Expand Down
30 changes: 30 additions & 0 deletions source/types/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ export type HttpMethod = 'get' | 'post' | 'put' | 'patch' | 'head' | 'delete';

export type Input = string | URL | Request;

export type UploadProgress = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me like we should just rename DownloadProgress to Progress or LoadProgress or something and then reuse it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to Progress

percent: number;
transferredBytes: number;

/**
Note: If it's not possible to retrieve the body size, it will be `0`.
*/
totalBytes: number;
};

export type DownloadProgress = {
percent: number;
transferredBytes: number;
Expand Down Expand Up @@ -188,6 +198,25 @@ export type KyOptions = {
*/
onDownloadProgress?: (progress: DownloadProgress, chunk: Uint8Array) => void;

/**
Upload progress event handler.

@param progress - Object containing upload progress information.

@example
```
import ky from 'ky';

const response = await ky.post('https://example.com/upload', {
body: new FormData(),
onUploadProgress: (progress) => {
console.log(`${progress.percent * 100}% - ${progress.transferredBytes} of ${progress.totalBytes} bytes`);
}
});
```
*/
onUploadProgress?: (progress: UploadProgress) => void;

/**
User-defined `fetch` function.
Has to be fully compatible with the [Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) standard.
Expand Down Expand Up @@ -287,6 +316,7 @@ export interface NormalizedOptions extends RequestInit { // eslint-disable-line
retry: RetryOptions;
prefixUrl: string;
onDownloadProgress: Options['onDownloadProgress'];
onUploadProgress: Options['onUploadProgress'];
}

export type {RetryOptions} from './retry.js';