From 51acfe9e6cfee7ae1ff73cfb0be6f47af510e17f Mon Sep 17 00:00:00 2001 From: Prakash Date: Fri, 29 May 2026 18:42:58 +0530 Subject: [PATCH 1/2] @uppy/tus: don't abort errored request (#6312) this fixes #6287 , another similar attempted fix which didn't got merged : #5683 in #5683 we discussed a more longer term solution but IMO that would be a breaking change as it would change the event contract AI Disclaimer : AI Used This fix is Manually Tested across **Chrome and Firefox** --- packages/@uppy/tus/package.json | 1 + packages/@uppy/tus/src/index.test.ts | 62 +++++++++++++++++++++++++++- packages/@uppy/tus/src/index.ts | 57 ++++++++++++++++++++++--- packages/@uppy/tus/vitest.config.ts | 25 +++++++++++ yarn.lock | 1 + 5 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 packages/@uppy/tus/vitest.config.ts diff --git a/packages/@uppy/tus/package.json b/packages/@uppy/tus/package.json index f34ad4930..a562867d2 100644 --- a/packages/@uppy/tus/package.json +++ b/packages/@uppy/tus/package.json @@ -44,6 +44,7 @@ "devDependencies": { "@uppy/core": "workspace:^", "jsdom": "^26.1.0", + "nock": "^13.1.0", "typescript": "^5.8.3", "vitest": "^3.2.4" }, diff --git a/packages/@uppy/tus/src/index.test.ts b/packages/@uppy/tus/src/index.test.ts index 206dffefd..3fc062f79 100644 --- a/packages/@uppy/tus/src/index.test.ts +++ b/packages/@uppy/tus/src/index.test.ts @@ -1,5 +1,6 @@ -import Core from '@uppy/core' -import { describe, expect, expectTypeOf, it } from 'vitest' +import Core, { type UppyEventMap } from '@uppy/core' +import nock from 'nock' +import { afterEach, describe, expect, expectTypeOf, it } from 'vitest' import Tus, { type TusBody } from './index.js' describe('Tus', () => { @@ -44,4 +45,61 @@ describe('Tus', () => { { xhr: XMLHttpRequest } | undefined >() }) + + describe('upload-error response', () => { + afterEach(() => { + nock.cleanAll() + }) + + it('sends the server response over the upload-error event', async () => { + nock('https://fake-endpoint.uppy.io') + .post('/files/') + .reply( + 403, + JSON.stringify({ + message: + 'File cannot be uploaded as the BIN content type is disallowed!', + status_code: 403, + }), + { 'Content-Type': 'application/json' }, + ) + + const core = new Core() + core.use(Tus, { + endpoint: 'https://fake-endpoint.uppy.io/files/', + // Avoid retrying so the failure surfaces immediately. + retryDelays: [], + }) + const id = core.addFile({ + type: 'application/octet-stream', + source: 'test', + name: 'test.bin', + data: new Blob([new Uint8Array(1024)]), + }) + + const event = new Promise< + Parameters['upload-error']> + >((resolve) => { + core.once('upload-error', (...args) => resolve(args)) + }) + + await Promise.all([ + core.upload().catch(() => { + // Core rejects the upload; we assert on the event/state instead. + }), + event.then(([, , response]) => { + expect(response?.status).toBe(403) + const { xhr } = response!.body! + expect(xhr).toBeInstanceOf(XMLHttpRequest) + expect(JSON.parse(xhr.responseText).message).toBe( + 'File cannot be uploaded as the BIN content type is disallowed!', + ) + }), + ]) + + // The response is also persisted on the file so it is available on the + // `complete` result and via `getFile`. + expect(core.getFile(id).response?.status).toBe(403) + }) + }) }) diff --git a/packages/@uppy/tus/src/index.ts b/packages/@uppy/tus/src/index.ts index 7eb00ebe7..fbbadd1a8 100644 --- a/packages/@uppy/tus/src/index.ts +++ b/packages/@uppy/tus/src/index.ts @@ -164,13 +164,28 @@ export default class Tus extends BasePlugin< * Clean up all references for a file's upload: the tus.Upload instance, * any events related to the file, and the Companion WebSocket connection. */ - resetUploaderReferences(fileID: string, opts?: { abort: boolean }): void { + resetUploaderReferences( + fileID: string, + opts?: { + /** Terminate the upload on the server (sends a `DELETE` request). */ + abort?: boolean + /** + * Abort the underlying request. Defaults to `true`. Set to `false` when + * the request has already completed (e.g. in the error handler), so the + * underlying `xhr` — and thus the server response — is preserved instead + * of being reset by `abort()`. + */ + abortRequest?: boolean + }, + ): void { const uploader = this.uploaders[fileID] if (uploader) { - uploader.abort() + if (opts?.abortRequest !== false) { + uploader.abort() - if (opts?.abort) { - uploader.abort(true) + if (opts?.abort) { + uploader.abort(true) + } } this.uploaders[fileID] = null @@ -219,6 +234,12 @@ export default class Tus extends BasePlugin< ): Promise { this.resetUploaderReferences(file.id) + // Captured in `onError` and forwarded to the `upload-error` event in the + // `.catch` below, so consumers can read the failing server response. + let errorResponse: + | Omit['response']>, 'uploadURL'> + | undefined + // Create a new tus upload return new Promise((resolve, reject) => { let queuedRequest: ReturnType @@ -291,6 +312,23 @@ export default class Tus extends BasePlugin< uploadOptions.onError = (err) => { this.uppy.log(err) + // tus-js-client only calls `onError` once it has given up retrying, so + // the request has already completed. Capture the server response (status + // + body) and forward it to the `upload-error` event and `file.response`, + // mirroring the shape emitted by `onSuccess`. + const originalResponse = (err as tus.DetailedError).originalResponse + if (originalResponse != null) { + errorResponse = { + status: originalResponse.getStatus(), + body: { + // We have to put `as XMLHttpRequest` because tus-js-client + // returns `any`, as the type differs in Node.js and the browser. + // In the browser it's always `XMLHttpRequest`. + xhr: originalResponse.getUnderlyingObject() as XMLHttpRequest, + } as unknown as B, + } + } + const xhr = (err as tus.DetailedError).originalRequest != null ? (err as tus.DetailedError).originalRequest.getUnderlyingObject() @@ -299,7 +337,11 @@ export default class Tus extends BasePlugin< err = new NetworkError(err, xhr) } - this.resetUploaderReferences(file.id) + // Do not abort the request here: it has already completed, and aborting + // it would reset the underlying `xhr` (status `0`, empty body) and + // discard the response we just captured. We still drop our references + // and remove the event listeners. + this.resetUploaderReferences(file.id, { abortRequest: false }) queuedRequest?.abort() if (typeof opts.onError === 'function') { @@ -516,7 +558,10 @@ export default class Tus extends BasePlugin< queuedRequest = this.requests.run(qRequest) }) }).catch((err) => { - this.uppy.emit('upload-error', file, err) + // `errorResponse` is captured in the `onError` handler above (the request + // is intentionally not aborted there), so the server response is still + // available here to forward to the `upload-error` event. + this.uppy.emit('upload-error', file, err, errorResponse) throw err }) } diff --git a/packages/@uppy/tus/vitest.config.ts b/packages/@uppy/tus/vitest.config.ts new file mode 100644 index 000000000..a4df19185 --- /dev/null +++ b/packages/@uppy/tus/vitest.config.ts @@ -0,0 +1,25 @@ +import { defineConfig } from 'vitest/config' + +// In the browser (where Uppy runs) tus-js-client uses its browser build, which +// accepts `Blob`/`File` and `XMLHttpRequest`. Under the default Node resolution +// Vitest would load the Node build, which only accepts `Buffer`/`Readable`. +// Alias to the browser build so jsdom tests exercise the same code path as +// production. +export default defineConfig({ + resolve: { + alias: { + 'tus-js-client': 'tus-js-client/lib.esm/browser/index.js', + }, + }, + test: { + environment: 'jsdom', + // Run with the document origin set to the upload endpoint so requests in + // tests are same-origin. Otherwise jsdom treats them as cross-origin and + // hides the response status (`xhr.status === 0`). + environmentOptions: { + jsdom: { + url: 'https://fake-endpoint.uppy.io', + }, + }, + }, +}) diff --git a/yarn.lock b/yarn.lock index b40d0901a..3af9a61ed 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9089,6 +9089,7 @@ __metadata: "@uppy/core": "workspace:^" "@uppy/utils": "workspace:^" jsdom: "npm:^26.1.0" + nock: "npm:^13.1.0" tus-js-client: "npm:^4.2.3" typescript: "npm:^5.8.3" vitest: "npm:^3.2.4" From ef370da2b3ec08960f22d0f8eedc2b87bb175405 Mon Sep 17 00:00:00 2001 From: Prakash Date: Fri, 29 May 2026 18:48:40 +0530 Subject: [PATCH 2/2] Revert "@uppy/tus: don't abort errored request" (#6313) Reverts transloadit/uppy#6312 --- packages/@uppy/tus/package.json | 1 - packages/@uppy/tus/src/index.test.ts | 62 +--------------------------- packages/@uppy/tus/src/index.ts | 57 +++---------------------- packages/@uppy/tus/vitest.config.ts | 25 ----------- yarn.lock | 1 - 5 files changed, 8 insertions(+), 138 deletions(-) delete mode 100644 packages/@uppy/tus/vitest.config.ts diff --git a/packages/@uppy/tus/package.json b/packages/@uppy/tus/package.json index a562867d2..f34ad4930 100644 --- a/packages/@uppy/tus/package.json +++ b/packages/@uppy/tus/package.json @@ -44,7 +44,6 @@ "devDependencies": { "@uppy/core": "workspace:^", "jsdom": "^26.1.0", - "nock": "^13.1.0", "typescript": "^5.8.3", "vitest": "^3.2.4" }, diff --git a/packages/@uppy/tus/src/index.test.ts b/packages/@uppy/tus/src/index.test.ts index 3fc062f79..206dffefd 100644 --- a/packages/@uppy/tus/src/index.test.ts +++ b/packages/@uppy/tus/src/index.test.ts @@ -1,6 +1,5 @@ -import Core, { type UppyEventMap } from '@uppy/core' -import nock from 'nock' -import { afterEach, describe, expect, expectTypeOf, it } from 'vitest' +import Core from '@uppy/core' +import { describe, expect, expectTypeOf, it } from 'vitest' import Tus, { type TusBody } from './index.js' describe('Tus', () => { @@ -45,61 +44,4 @@ describe('Tus', () => { { xhr: XMLHttpRequest } | undefined >() }) - - describe('upload-error response', () => { - afterEach(() => { - nock.cleanAll() - }) - - it('sends the server response over the upload-error event', async () => { - nock('https://fake-endpoint.uppy.io') - .post('/files/') - .reply( - 403, - JSON.stringify({ - message: - 'File cannot be uploaded as the BIN content type is disallowed!', - status_code: 403, - }), - { 'Content-Type': 'application/json' }, - ) - - const core = new Core() - core.use(Tus, { - endpoint: 'https://fake-endpoint.uppy.io/files/', - // Avoid retrying so the failure surfaces immediately. - retryDelays: [], - }) - const id = core.addFile({ - type: 'application/octet-stream', - source: 'test', - name: 'test.bin', - data: new Blob([new Uint8Array(1024)]), - }) - - const event = new Promise< - Parameters['upload-error']> - >((resolve) => { - core.once('upload-error', (...args) => resolve(args)) - }) - - await Promise.all([ - core.upload().catch(() => { - // Core rejects the upload; we assert on the event/state instead. - }), - event.then(([, , response]) => { - expect(response?.status).toBe(403) - const { xhr } = response!.body! - expect(xhr).toBeInstanceOf(XMLHttpRequest) - expect(JSON.parse(xhr.responseText).message).toBe( - 'File cannot be uploaded as the BIN content type is disallowed!', - ) - }), - ]) - - // The response is also persisted on the file so it is available on the - // `complete` result and via `getFile`. - expect(core.getFile(id).response?.status).toBe(403) - }) - }) }) diff --git a/packages/@uppy/tus/src/index.ts b/packages/@uppy/tus/src/index.ts index fbbadd1a8..7eb00ebe7 100644 --- a/packages/@uppy/tus/src/index.ts +++ b/packages/@uppy/tus/src/index.ts @@ -164,28 +164,13 @@ export default class Tus extends BasePlugin< * Clean up all references for a file's upload: the tus.Upload instance, * any events related to the file, and the Companion WebSocket connection. */ - resetUploaderReferences( - fileID: string, - opts?: { - /** Terminate the upload on the server (sends a `DELETE` request). */ - abort?: boolean - /** - * Abort the underlying request. Defaults to `true`. Set to `false` when - * the request has already completed (e.g. in the error handler), so the - * underlying `xhr` — and thus the server response — is preserved instead - * of being reset by `abort()`. - */ - abortRequest?: boolean - }, - ): void { + resetUploaderReferences(fileID: string, opts?: { abort: boolean }): void { const uploader = this.uploaders[fileID] if (uploader) { - if (opts?.abortRequest !== false) { - uploader.abort() + uploader.abort() - if (opts?.abort) { - uploader.abort(true) - } + if (opts?.abort) { + uploader.abort(true) } this.uploaders[fileID] = null @@ -234,12 +219,6 @@ export default class Tus extends BasePlugin< ): Promise { this.resetUploaderReferences(file.id) - // Captured in `onError` and forwarded to the `upload-error` event in the - // `.catch` below, so consumers can read the failing server response. - let errorResponse: - | Omit['response']>, 'uploadURL'> - | undefined - // Create a new tus upload return new Promise((resolve, reject) => { let queuedRequest: ReturnType @@ -312,23 +291,6 @@ export default class Tus extends BasePlugin< uploadOptions.onError = (err) => { this.uppy.log(err) - // tus-js-client only calls `onError` once it has given up retrying, so - // the request has already completed. Capture the server response (status - // + body) and forward it to the `upload-error` event and `file.response`, - // mirroring the shape emitted by `onSuccess`. - const originalResponse = (err as tus.DetailedError).originalResponse - if (originalResponse != null) { - errorResponse = { - status: originalResponse.getStatus(), - body: { - // We have to put `as XMLHttpRequest` because tus-js-client - // returns `any`, as the type differs in Node.js and the browser. - // In the browser it's always `XMLHttpRequest`. - xhr: originalResponse.getUnderlyingObject() as XMLHttpRequest, - } as unknown as B, - } - } - const xhr = (err as tus.DetailedError).originalRequest != null ? (err as tus.DetailedError).originalRequest.getUnderlyingObject() @@ -337,11 +299,7 @@ export default class Tus extends BasePlugin< err = new NetworkError(err, xhr) } - // Do not abort the request here: it has already completed, and aborting - // it would reset the underlying `xhr` (status `0`, empty body) and - // discard the response we just captured. We still drop our references - // and remove the event listeners. - this.resetUploaderReferences(file.id, { abortRequest: false }) + this.resetUploaderReferences(file.id) queuedRequest?.abort() if (typeof opts.onError === 'function') { @@ -558,10 +516,7 @@ export default class Tus extends BasePlugin< queuedRequest = this.requests.run(qRequest) }) }).catch((err) => { - // `errorResponse` is captured in the `onError` handler above (the request - // is intentionally not aborted there), so the server response is still - // available here to forward to the `upload-error` event. - this.uppy.emit('upload-error', file, err, errorResponse) + this.uppy.emit('upload-error', file, err) throw err }) } diff --git a/packages/@uppy/tus/vitest.config.ts b/packages/@uppy/tus/vitest.config.ts deleted file mode 100644 index a4df19185..000000000 --- a/packages/@uppy/tus/vitest.config.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { defineConfig } from 'vitest/config' - -// In the browser (where Uppy runs) tus-js-client uses its browser build, which -// accepts `Blob`/`File` and `XMLHttpRequest`. Under the default Node resolution -// Vitest would load the Node build, which only accepts `Buffer`/`Readable`. -// Alias to the browser build so jsdom tests exercise the same code path as -// production. -export default defineConfig({ - resolve: { - alias: { - 'tus-js-client': 'tus-js-client/lib.esm/browser/index.js', - }, - }, - test: { - environment: 'jsdom', - // Run with the document origin set to the upload endpoint so requests in - // tests are same-origin. Otherwise jsdom treats them as cross-origin and - // hides the response status (`xhr.status === 0`). - environmentOptions: { - jsdom: { - url: 'https://fake-endpoint.uppy.io', - }, - }, - }, -}) diff --git a/yarn.lock b/yarn.lock index 3af9a61ed..b40d0901a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9089,7 +9089,6 @@ __metadata: "@uppy/core": "workspace:^" "@uppy/utils": "workspace:^" jsdom: "npm:^26.1.0" - nock: "npm:^13.1.0" tus-js-client: "npm:^4.2.3" typescript: "npm:^5.8.3" vitest: "npm:^3.2.4"