diff --git a/.changeset/polite-cougars-itch.md b/.changeset/polite-cougars-itch.md new file mode 100644 index 000000000..983456042 --- /dev/null +++ b/.changeset/polite-cougars-itch.md @@ -0,0 +1,9 @@ +--- +"@uppy/aws-s3": patch +"@uppy/core": patch +"@uppy/tus": patch +"@uppy/utils": patch +"@uppy/xhr-upload": patch +--- + +Fix: Move completed uploads exclusion logic into uploaders. This fixes the problem where postprocessors would not run for already uploaded files. diff --git a/packages/@uppy/aws-s3/src/index.ts b/packages/@uppy/aws-s3/src/index.ts index 9f0dde6ae..d248f4c5b 100644 --- a/packages/@uppy/aws-s3/src/index.ts +++ b/packages/@uppy/aws-s3/src/index.ts @@ -16,7 +16,7 @@ import type { import { createAbortError, filterFilesToEmitUploadStarted, - filterNonFailedFiles, + filterFilesToUpload, getAllowedMetaFields, RateLimitedQueue, } from '@uppy/utils' @@ -938,7 +938,7 @@ export default class AwsS3Multipart< if (fileIDs.length === 0) return undefined const files = this.uppy.getFilesByIds(fileIDs) - const filesFiltered = filterNonFailedFiles(files) + const filesFiltered = filterFilesToUpload(files) const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered) this.uppy.emit('upload-start', filesToEmit) diff --git a/packages/@uppy/core/src/Uppy.test.ts b/packages/@uppy/core/src/Uppy.test.ts index 758864d12..cf2a5530d 100644 --- a/packages/@uppy/core/src/Uppy.test.ts +++ b/packages/@uppy/core/src/Uppy.test.ts @@ -2609,7 +2609,7 @@ describe('src/Core', () => { expect(infoVisibleEvent.mock.calls.length).toEqual(1) }) - it('should set an info message to be displayed for a period of time before hiding', (done) => { + it('should set an info message to be displayed for a period of time before hiding', async () => { const infoVisibleEvent = vi.fn() const infoHiddenEvent = vi.fn() const core = new Core() @@ -2618,12 +2618,10 @@ describe('src/Core', () => { core.info('This is the message', 'info', 100) expect(infoHiddenEvent.mock.calls.length).toEqual(0) - setTimeout(() => { - expect(infoHiddenEvent.mock.calls.length).toEqual(1) - expect(core.getState().info).toEqual([]) - // @ts-ignore - done() - }, 110) + + await new Promise((resolve) => setTimeout(resolve, 110)) + expect(infoHiddenEvent.mock.calls.length).toEqual(1) + expect(core.getState().info).toEqual([]) }) it('should hide an info message', () => { diff --git a/packages/@uppy/core/src/Uppy.ts b/packages/@uppy/core/src/Uppy.ts index b09add1db..6f13065b2 100644 --- a/packages/@uppy/core/src/Uppy.ts +++ b/packages/@uppy/core/src/Uppy.ts @@ -2233,9 +2233,6 @@ export class Uppy< ] try { for (let step = currentUpload.step || 0; step < steps.length; step++) { - if (!currentUpload) { - break - } const fn = steps[step] this.setState({ @@ -2248,13 +2245,7 @@ export class Uppy< }, }) - // when restoring (e.g. using golden retriever), we don't need to re-upload already successfully uploaded files - // so let's exclude them here: - // https://github.com/transloadit/uppy/issues/5930 - const fileIDs = currentUpload.fileIDs.filter((fileID) => { - const file = this.getFile(fileID) - return !file.progress.uploadComplete - }) + const { fileIDs } = currentUpload // TODO give this the `updatedUpload` object as its only parameter maybe? // Otherwise when more metadata may be added to the upload this would keep getting more parameters @@ -2262,6 +2253,9 @@ export class Uppy< // Update currentUpload value in case it was modified asynchronously. currentUpload = getCurrentUpload() + if (!currentUpload) { + break + } } } catch (err) { this.#removeUpload(uploadID) diff --git a/packages/@uppy/golden-retriever/src/goldenRetriever.browser.test.ts b/packages/@uppy/golden-retriever/src/goldenRetriever.browser.test.ts index 1ea91c3c5..3e351a8bf 100644 --- a/packages/@uppy/golden-retriever/src/goldenRetriever.browser.test.ts +++ b/packages/@uppy/golden-retriever/src/goldenRetriever.browser.test.ts @@ -20,7 +20,9 @@ function createUppy( const root = document.createElement('div') document.body.appendChild(root) - return new Uppy().use(Dashboard, { + return new Uppy({ + debug: true, + }).use(Dashboard, { target: root, inline: true, ...opts, @@ -36,9 +38,11 @@ beforeEach(async () => { document.body.innerHTML = '' }) +let fileIndex = 0 + const createMockFile = ({ size, - name = `${Date.now()}.txt`, + name = `${fileIndex++}.txt`, type = 'text/plain', }: { name?: string @@ -99,7 +103,9 @@ describe('Golden retriever', () => { requestAt += 1 return HttpResponse.json({}) } - // never reply to subsequent requests (leave them hanging) + // fail subsequent requests + requestAt += 1 + return HttpResponse.json({}, { status: 400 }) }), ) @@ -117,10 +123,15 @@ describe('Golden retriever', () => { uppy.once('upload-success', () => resolve()), ) + const uploadCompletePromise = new Promise((resolve) => + uppy.once('complete', () => resolve()), + ) + // Start the upload await page.getByRole('button', { name: 'Upload 2 files' }).click() await uploadFirstFileCompletePromise + await uploadCompletePromise // reload page and recreate Uppy instance uppy = createUppy({ withPageReload: true }) @@ -140,7 +151,8 @@ describe('Golden retriever', () => { return HttpResponse.json({}) } // don't allow more than 1 request - return new HttpResponse({ status: 400 }) + requestAt += 1 + return HttpResponse.json({}, { status: 400 }) }), ) @@ -152,6 +164,7 @@ describe('Golden retriever', () => { .toBeVisible() expect(uppy.getFiles().length).toBe(2) + expect(requestAt).toBe(1) // only the first file upload should have happened so far }) test('Should not clean up files upon completion if there were failed uploads and it should only make the failed file a ghost', async ({ diff --git a/packages/@uppy/tus/src/index.ts b/packages/@uppy/tus/src/index.ts index 1b9695ed4..6db9c2e9b 100644 --- a/packages/@uppy/tus/src/index.ts +++ b/packages/@uppy/tus/src/index.ts @@ -10,7 +10,7 @@ import type { import { BasePlugin, EventManager } from '@uppy/core' import { filterFilesToEmitUploadStarted, - filterNonFailedFiles, + filterFilesToUpload, getAllowedMetaFields, hasProperty, isNetworkError, @@ -563,7 +563,7 @@ export default class Tus extends BasePlugin< } async #uploadFiles(files: UppyFile[]) { - const filesFiltered = filterNonFailedFiles(files) + const filesFiltered = filterFilesToUpload(files) const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered) this.uppy.emit('upload-start', filesToEmit) diff --git a/packages/@uppy/utils/src/fileFilters.ts b/packages/@uppy/utils/src/fileFilters.ts index 415bdef7a..3a1dabf9f 100644 --- a/packages/@uppy/utils/src/fileFilters.ts +++ b/packages/@uppy/utils/src/fileFilters.ts @@ -1,12 +1,17 @@ import type { UppyFile } from './UppyFile.js' -export function filterNonFailedFiles( +const hasError = (file: UppyFile) => 'error' in file && !!file.error + +// We don't need to re-upload already successfully uploaded files +// so let's exclude them here: +// https://github.com/transloadit/uppy/issues/5930 +// This happens for example when restoring a partially finished session (e.g. using golden retriever). +const isCompleted = (file: UppyFile) => file.progress.uploadComplete + +export function filterFilesToUpload( files: UppyFile[], ): UppyFile[] { - const hasError = (file: UppyFile): boolean => - 'error' in file && !!file.error - - return files.filter((file) => !hasError(file)) + return files.filter((file) => !hasError(file) && !isCompleted(file)) } // Don't double-emit upload-started for Golden Retriever-restored files that were already started diff --git a/packages/@uppy/utils/src/index.ts b/packages/@uppy/utils/src/index.ts index 2d4e84952..467a89296 100644 --- a/packages/@uppy/utils/src/index.ts +++ b/packages/@uppy/utils/src/index.ts @@ -37,7 +37,7 @@ export { default as fetchWithNetworkError } from './fetchWithNetworkError.js' export { filterFilesToEmitUploadStarted, - filterNonFailedFiles, + filterFilesToUpload, } from './fileFilters.js' export { default as findAllDOMElements } from './findAllDOMElements.js' diff --git a/packages/@uppy/xhr-upload/src/index.ts b/packages/@uppy/xhr-upload/src/index.ts index f817181b0..a4a461f75 100644 --- a/packages/@uppy/xhr-upload/src/index.ts +++ b/packages/@uppy/xhr-upload/src/index.ts @@ -13,7 +13,7 @@ import { type FetcherOptions, fetcher, filterFilesToEmitUploadStarted, - filterNonFailedFiles, + filterFilesToUpload, getAllowedMetaFields, internalRateLimitedQueue, isNetworkError, @@ -535,7 +535,7 @@ export default class XHRUpload< this.uppy.log('[XHRUpload] Uploading...') const files = this.uppy.getFilesByIds(fileIDs) - const filesFiltered = filterNonFailedFiles(files) + const filesFiltered = filterFilesToUpload(files) const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered) this.uppy.emit('upload-start', filesToEmit)