From f881fa031eb73bc7fd2fe13fac646d35b2510bc1 Mon Sep 17 00:00:00 2001 From: Merlijn Vos Date: Thu, 13 Mar 2025 09:46:24 +0100 Subject: [PATCH] @uppy/core: make upload() idempotent (#5677) Before Add a file Call uppy.upload() but make it fail (set throttling to offline in your browser) Call uppy.upload() again but no throttling Events are not fired, endless uploading state. You must call retryAll() instead but it's better DX if you can simply call upload() again and let us figure it out. After upload() behaves like retryAll() when errors occurred in a backwards compatible way. What if an upload fails and you also add a new file? Backwards compatible behaviour similar to how it currently works when using the dashboard. In the dashboard you can only click the retry button and once that upload is done you can click upload again to upload the new files. When a previous upload partially failed, you add a new file, and call upload() this PR makes sure two uploads are done in a row. That does mean you get the 'complete' event twice. --- packages/@uppy/core/src/Uppy.test.ts | 83 +++++++++++++++++++++------- packages/@uppy/core/src/Uppy.ts | 41 +++++++++++++- 2 files changed, 102 insertions(+), 22 deletions(-) diff --git a/packages/@uppy/core/src/Uppy.test.ts b/packages/@uppy/core/src/Uppy.test.ts index 86815dba6..e03e5e339 100644 --- a/packages/@uppy/core/src/Uppy.test.ts +++ b/packages/@uppy/core/src/Uppy.test.ts @@ -821,27 +821,6 @@ describe('src/Core', () => { uploadStarted: null, }) }) - - it('should report an error if post-processing a file fails', () => { - const core = new Core() - - core.addFile({ - source: 'vi', - name: 'foo.jpg', - type: 'image/jpeg', - data: testImage, - }) - - const fileId = Object.keys(core.getState().files)[0] - const file = core.getFile(fileId) - core.emit('error', new Error('foooooo'), file) - - expect(core.getState().error).toEqual('foooooo') - - expect(core.upload()).resolves.toMatchObject({ - failed: [{ name: 'foo.jpg' }], - }) - }) }) describe('uploaders', () => { @@ -1357,6 +1336,68 @@ describe('src/Core', () => { }) await expect(core.upload()).resolves.toBeDefined() }) + + it('upload() is idempotent when called multiple times', async () => { + const onUpload = vi.fn() + const onRetryAll = vi.fn() + const onUploadError = vi.fn() + const core = new Core() + let hasError = false + + core.addUploader((fileIDs) => { + fileIDs.forEach((fileID) => { + const file = core.getFile(fileID) + if (!hasError) { + // @ts-ignore + core.emit('upload-error', file, new Error('foo')) + hasError = true + } + }) + return Promise.resolve() + }) + core.on('upload', onUpload) + core.on('retry-all', onRetryAll) + core.on('upload-error', onUploadError) + + const firstFileID = core.addFile({ + source: 'vi', + name: 'foo.jpg', + type: 'image/jpeg', + data: testImage, + }) + // First time 'upload' and 'upload-error' should be emitted + await core.upload() + expect(onRetryAll).not.toHaveBeenCalled() + expect(onUpload).toHaveBeenCalled() + expect(onUploadError).toHaveBeenCalled() + + // Reset counters + onRetryAll.mockReset() + onUpload.mockReset() + onUploadError.mockReset() + + // One failed file in memory but we add a new one before uploading + core.addFile({ + source: 'vi', + name: 'bar.jpg', + type: 'image/jpeg', + data: testImage, + }) + const onComplete = vi.fn() + core.on('complete', onComplete) + // Second time two uploads should happen back-to-back. + // First to retry the failed files, which will emit events, and the second upload + // for the new files, which also emits events. + await core.upload() + expect(onRetryAll).toBeCalledTimes(1) + expect(onUpload).toBeCalledTimes(2) + expect(onUploadError).toBeCalledTimes(0) + expect(onComplete).toBeCalledTimes(2) + const result = onComplete.mock.calls[0][0] + expect(result.successful?.length).toBe(1) + expect(result.failed?.length).toBe(0) + expect(result.successful?.[0].id).toBe(firstFileID) + }) }) describe('removing a file', () => { diff --git a/packages/@uppy/core/src/Uppy.ts b/packages/@uppy/core/src/Uppy.ts index 21cc3b3e2..f9166746b 100644 --- a/packages/@uppy/core/src/Uppy.ts +++ b/packages/@uppy/core/src/Uppy.ts @@ -2241,13 +2241,52 @@ export class Uppy< /** * Start an upload for all the files that are not currently being uploaded. */ - upload(): Promise> | undefined> { + async upload(): Promise> | undefined> { if (!this.#plugins['uploader']?.length) { this.log('No uploader type plugins are used', 'warning') } let { files } = this.getState() + // Check if we have files with errors (for retry behavior) + const filesToRetry = Object.keys(files).filter( + (fileID) => files[fileID].error, + ) + const hasFilesToRetry = filesToRetry.length > 0 + + // If we have files to retry, behave like retryAll() and ignore any new files + if (hasFilesToRetry) { + const updatedFiles = { ...files } + filesToRetry.forEach((fileID) => { + updatedFiles[fileID] = { + ...updatedFiles[fileID], + isPaused: false, + error: null, + } + }) + + this.setState({ + files: updatedFiles, + error: null, + }) + + this.emit('retry-all', Object.values(updatedFiles)) + + const uploadID = this.#createUpload(filesToRetry, { + forceAllowNewUpload: true, // create new upload even if allowNewUpload: false + }) + const result = await this.#runUpload(uploadID) + const hasNewFiles = this.getFiles().filter( + (file) => file.progress.uploadStarted == null, + ) + + if (!hasNewFiles) { + return result + } + files = this.getState().files + } + + // If no files to retry, proceed with original upload() behavior for new files const onBeforeUploadResult = this.opts.onBeforeUpload(files) if (onBeforeUploadResult === false) {