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

Conversation

jadedevin13
Copy link

No description provided.

@jadedevin13 jadedevin13 closed this Sep 6, 2024
Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

We will also need tests.

// 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?

@@ -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

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),
					});

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.

@jadedevin13 jadedevin13 reopened this Sep 7, 2024
Comment on lines +446 to +453
Notes:
- This option requires environment support for `ReadableStream`. If streams are not supported, an error will be thrown.
- The `body` of the request must be set for this option to have an effect.
- The total size calculation is an approximation for certain types of bodies (e.g., `FormData`).
- For `FormData` bodies, the size calculation includes an estimation for multipart boundaries and field names.
- If the total size cannot be determined, `totalBytes` will be 0, and `percent` will remain at 0 throughout the upload and will be 1 once upload is finished.

This feature is useful for tracking the progress of large file uploads or when sending substantial amounts of data to a server.
Copy link
Owner

Choose a reason for hiding this comment

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

The TS doc comments and readme should be in sync.

@RomainLanz
Copy link

Hey there! 👋🏻

Just checking in—are there any blockers I can help with to move this PR forward for merging?

const jsonString = JSON.stringify(body);
return new TextEncoder().encode(jsonString).length;
} catch (error) {
console.warn('Unable to stringify object:', error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a library, it's generally considered bad practice for Ky to output non-errors to the console by default.

@sholladay
Copy link
Collaborator

I think this is ready to go once that warning is removed.

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

Successfully merging this pull request may close these issues.

5 participants