Skip to content

Add abort signal to upload#373

Merged
dajimenezriv-internxt merged 6 commits intomasterfrom
add-abort-signal-to-upload
Mar 6, 2026
Merged

Add abort signal to upload#373
dajimenezriv-internxt merged 6 commits intomasterfrom
add-abort-signal-to-upload

Conversation

@dajimenezriv-internxt
Copy link
Contributor

@dajimenezriv-internxt dajimenezriv-internxt commented Mar 5, 2026

What

  1. The main change is adding the abortSignal to the requests of upload file.
  2. I found that the endpoint of uploading files accepts more than one file at the same time but we are just doing 1 per request and it has been like this for a long time. So I removed that dynamic array of files creation and hardcoded it to just 1, since I think it's easier to read.
  3. Add async and await to add the functions to the stack trace. Add abort signal to upload inxt-js#106 (comment)

const encryptFileMock = vi.fn();
const uploadFileMock = vi.fn().mockReturnValue(fakeHash);

try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this try catch since if the test throws an error (which happen to me) the test already fails and gives a more detailed error.

Copy link
Member

@sg-gs sg-gs left a comment

Choose a reason for hiding this comment

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

Shouldn't be the signal parameter optional? Otherwise all the clients are forced to adapt to a parameter that is commonly optional

}
totalSize += size;
async startUpload(bucketId: string, fileSize: number, signal: AbortSignal, parts = 1): Promise<StartUploadResponse> {
if (fileSize < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that even <= 0 should be invalid also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I will change it

* @param index
* @param shards
*/
private static finishUpload(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was static to be able to be called finishUpload too, however, I think it's easier to read if we use different names (this one just makes the request), and by making it non static we can acces the client, appDetails and auth directly from this function to make the references more direct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked also if any project was using the static version of finishUpload but seems that nothing is using it: https://github.com/search?q=org%3Ainternxt%20finishUpload&type=code.

@dajimenezriv-internxt dajimenezriv-internxt merged commit b54c39b into master Mar 6, 2026
1 check passed
@dajimenezriv-internxt dajimenezriv-internxt deleted the add-abort-signal-to-upload branch March 6, 2026 09:03
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.

2 participants