Add onUploadProgress option #632
Conversation
source/core/Ky.ts
Outdated
| // 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.'); |
There was a problem hiding this comment.
This should instead say something about request.duplex not being supported.
There was a problem hiding this comment.
This is a copy of the message in onDownloadProgress. Should I change that as well?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Changed to 'Request streams are not supported in your environment. The duplex option for Request is not available.'. Would this error message be enough?
source/types/options.ts
Outdated
| @@ -12,6 +12,16 @@ export type HttpMethod = 'get' | 'post' | 'put' | 'patch' | 'head' | 'delete'; | |||
|
|
|||
| export type Input = string | URL | Request; | |||
|
|
|||
| export type UploadProgress = { | |||
There was a problem hiding this comment.
Seems to me like we should just rename DownloadProgress to Progress or LoadProgress or something and then reuse it.
source/core/Ky.ts
Outdated
| body: this._wrapBodyWithUploadProgress(originalBody, totalBytes, this._options.onUploadProgress), | ||
| headers: this.request.headers, | ||
| method: this.request.method, | ||
| signal: this.request.signal, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
No reason. Changed it to
this.request
= new globalThis.Request(this._input, {
...this._options,
body: this._streamRequest(
originalBody, totalBytes, this._options.onUploadProgress),
});
source/core/Ky.ts
Outdated
| return 0; // Default case, unable to determine size | ||
| } | ||
|
|
||
| protected _wrapBodyWithUploadProgress( |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Changed to _streamRequest and _streamResponse
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
The TS doc comments and readme should be in sync.
|
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); |
There was a problem hiding this comment.
As a library, it's generally considered bad practice for Ky to output non-errors to the console by default.
|
I think this is ready to go once that warning is removed. |
No description provided.