@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

@@ -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', () => {

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<NonNullable<UploadResult<M, B>> | undefined> {
+  async upload(): Promise<NonNullable<UploadResult<M, B>> | 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) {