proposal: Cancel assemblies optional (#3575)

* change cancel logic to make canceling assemblies optional possibly fixes #3547 * add forgotten file * rewrite to reason='user' * try to fix crash * change reason to unmount * Apply suggestions from code review * add close+unmount in more code examples also fix rule of hooks issue * improve reason docs * add tests * add options also to reset * update doc * also prevent canceling uploads on unmount * Update website/src/docs/transloadit.md Co-authored-by: Merlijn Vos <[email protected]> * remove conflicting file Co-authored-by: Merlijn Vos <[email protected]>
transloadit · May 5, 2022 · 8ed2105 · 8ed2105
1 parent d42badf
commit 8ed2105
Show file tree

Hide file tree

Showing 15 changed files with 106 additions and 80 deletions.
diff --git a/examples/react-example/App.js b/examples/react-example/App.js
@@ -25,8 +25,8 @@ module.exports = class App extends React.Component {
   }
 
   componentWillUnmount () {
-    this.uppy.close()
-    this.uppy2.close()
+    this.uppy.close({ reason: 'unmount' })
+    this.uppy2.close({ reason: 'unmount' })
   }
 
   handleModalClick () {

diff --git a/packages/@uppy/aws-s3-multipart/src/index.js b/packages/@uppy/aws-s3-multipart/src/index.js
@@ -227,9 +227,11 @@ module.exports = class AwsS3Multipart extends BasePlugin {
         resolve(`upload ${removed.id} was removed`)
       })
 
-      this.onCancelAll(file.id, () => {
-        queuedRequest.abort()
-        this.resetUploaderReferences(file.id, { abort: true })
+      this.onCancelAll(file.id, ({ reason } = {}) => {
+        if (reason === 'user') {
+          queuedRequest.abort()
+          this.resetUploaderReferences(file.id, { abort: true })
+        }
         resolve(`upload ${file.id} was canceled`)
       })
 
@@ -346,10 +348,12 @@ module.exports = class AwsS3Multipart extends BasePlugin {
         socket.send('pause', {})
       })
 
-      this.onCancelAll(file.id, () => {
-        queuedRequest.abort()
-        socket.send('cancel', {})
-        this.resetUploaderReferences(file.id)
+      this.onCancelAll(file.id, ({ reason } = {}) => {
+        if (reason === 'user') {
+          queuedRequest.abort()
+          socket.send('cancel', {})
+          this.resetUploaderReferences(file.id)
+        }
         resolve(`upload ${file.id} was canceled`)
       })
 
@@ -463,10 +467,10 @@ module.exports = class AwsS3Multipart extends BasePlugin {
     })
   }
 
-  onCancelAll (fileID, cb) {
-    this.uploaderEvents[fileID].on('cancel-all', () => {
+  onCancelAll (fileID, eventHandler) {
+    this.uploaderEvents[fileID].on('cancel-all', (...args) => {
       if (!this.uppy.getFile(fileID)) return
-      cb()
+      eventHandler(...args)
     })
   }
 

diff --git a/packages/@uppy/aws-s3/src/MiniXHRUpload.js b/packages/@uppy/aws-s3/src/MiniXHRUpload.js
@@ -102,8 +102,8 @@ module.exports = class MiniXHRUpload {
   }
 
   #addEventHandlerIfFileStillExists (eventName, fileID, eventHandler) {
-    this.uploaderEvents[fileID].on(eventName, () => {
-      if (this.uppy.getFile(fileID)) eventHandler()
+    this.uploaderEvents[fileID].on(eventName, (...args) => {
+      if (this.uppy.getFile(fileID)) eventHandler(...args)
     })
   }
 
@@ -234,8 +234,10 @@ module.exports = class MiniXHRUpload {
         reject(new Error('File removed'))
       })
 
-      this.#addEventHandlerIfFileStillExists('cancel-all', file.id, () => {
-        queuedRequest.abort()
+      this.#addEventHandlerIfFileStillExists('cancel-all', file.id, ({ reason } = {}) => {
+        if (reason === 'user') {
+          queuedRequest.abort()
+        }
         reject(new Error('Upload cancelled'))
       })
     })
@@ -283,9 +285,11 @@ module.exports = class MiniXHRUpload {
         resolve(`upload ${file.id} was removed`)
       })
 
-      this.#addEventHandlerIfFileStillExists('cancel-all', file.id, () => {
-        socket.send('cancel', {})
-        queuedRequest.abort()
+      this.#addEventHandlerIfFileStillExists('cancel-all', file.id, ({ reason } = {}) => {
+        if (reason === 'user') {
+          socket.send('cancel', {})
+          queuedRequest.abort()
+        }
         resolve(`upload ${file.id} was canceled`)
       })
 

diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js
@@ -807,21 +807,24 @@ class Uppy {
     return this.#runUpload(uploadID)
   }
 
-  cancelAll () {
-    this.emit('cancel-all')
+  cancelAll ({ reason = 'user' } = {}) {
+    this.emit('cancel-all', { reason })
 
-    const { files } = this.getState()
+    // Only remove existing uploads if user is canceling
+    if (reason === 'user') {
+      const { files } = this.getState()
 
-    const fileIDs = Object.keys(files)
-    if (fileIDs.length) {
-      this.removeFiles(fileIDs, 'cancel-all')
-    }
+      const fileIDs = Object.keys(files)
+      if (fileIDs.length) {
+        this.removeFiles(fileIDs, 'cancel-all')
+      }
 
-    this.setState({
-      totalProgress: 0,
-      error: null,
-      recoveredState: null,
-    })
+      this.setState({
+        totalProgress: 0,
+        error: null,
+        recoveredState: null,
+      })
+    }
   }
 
   retryUpload (fileID) {
@@ -838,8 +841,9 @@ class Uppy {
     return this.#runUpload(uploadID)
   }
 
-  reset () {
-    this.cancelAll()
+  // todo remove in next major. what is the point of the reset method when we have cancelAll or vice versa?
+  reset (...args) {
+    this.cancelAll(...args)
   }
 
   logout () {
@@ -1234,10 +1238,10 @@ class Uppy {
   /**
    * Uninstall all plugins and close down this Uppy instance.
    */
-  close () {
+  close ({ reason } = {}) {
     this.log(`Closing Uppy instance ${this.opts.id}: removing all files and uninstalling plugins`)
 
-    this.reset()
+    this.cancelAll({ reason })
 
     this.#storeUnsubscribe()
 

diff --git a/packages/@uppy/core/src/Uppy.test.js b/packages/@uppy/core/src/Uppy.test.js
@@ -223,7 +223,7 @@ describe('src/Core', () => {
 
     core.reset()
 
-    expect(coreCancelEventMock.mock.calls.length).toEqual(1)
+    expect(coreCancelEventMock).toHaveBeenCalledWith({ reason: 'user' }, undefined, undefined, undefined, undefined, undefined)
     expect(coreStateUpdateEventMock.mock.calls.length).toEqual(2)
     expect(coreStateUpdateEventMock.mock.calls[1][1]).toEqual({
       capabilities: { individualCancellation: true, uploadProgress: true, resumableUploads: false },
@@ -285,7 +285,7 @@ describe('src/Core', () => {
 
     core.close()
 
-    expect(coreCancelEventMock.mock.calls.length).toEqual(1)
+    expect(coreCancelEventMock).toHaveBeenCalledWith({ reason: 'user' }, undefined, undefined, undefined, undefined, undefined)
     expect(coreStateUpdateEventMock.mock.calls.length).toEqual(1)
     expect(coreStateUpdateEventMock.mock.calls[0][1]).toEqual({
       capabilities: { individualCancellation: true, uploadProgress: true, resumableUploads: false },

diff --git a/packages/@uppy/react/src/useUppy.js b/packages/@uppy/react/src/useUppy.js
@@ -17,7 +17,7 @@ module.exports = function useUppy (factory) {
 
   useEffect(() => {
     return () => {
-      uppy.current.close()
+      uppy.current.close({ reason: 'unmount' })
     }
   }, [])
 

diff --git a/packages/@uppy/transloadit/src/index.js b/packages/@uppy/transloadit/src/index.js
@@ -398,19 +398,18 @@ module.exports = class Transloadit extends BasePlugin {
   /**
    * When all files are removed, cancel in-progress Assemblies.
    */
-  #onCancelAll = () => {
-    const { uploadsAssemblies } = this.getPluginState()
-
-    const assemblyIDs = Object.values(uploadsAssemblies).flat(1)
+  #onCancelAll = async ({ reason } = {}) => {
+    try {
+      if (reason !== 'user') return
 
-    const cancelPromises = assemblyIDs.map((assemblyID) => {
-      const assembly = this.getAssembly(assemblyID)
-      return this.#cancelAssembly(assembly)
-    })
+      const { uploadsAssemblies } = this.getPluginState()
+      const assemblyIDs = Object.values(uploadsAssemblies).flat(1)
+      const assemblies = assemblyIDs.map((assemblyID) => this.getAssembly(assemblyID))
 
-    Promise.all(cancelPromises).catch((err) => {
+      await Promise.all(assemblies.map((assembly) => this.#cancelAssembly(assembly)))
+    } catch (err) {
       this.uppy.log(err)
-    })
+    }
   }
 
   /**

diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js
@@ -388,9 +388,11 @@ module.exports = class Tus extends BasePlugin {
         upload.abort()
       })
 
-      this.onCancelAll(file.id, () => {
-        queuedRequest.abort()
-        this.resetUploaderReferences(file.id, { abort: !!upload.url })
+      this.onCancelAll(file.id, ({ reason } = {}) => {
+        if (reason === 'user') {
+          queuedRequest.abort()
+          this.resetUploaderReferences(file.id, { abort: !!upload.url })
+        }
         resolve(`upload ${file.id} was canceled`)
       })
 
@@ -501,10 +503,12 @@ module.exports = class Tus extends BasePlugin {
         socket.send('pause', {})
       })
 
-      this.onCancelAll(file.id, () => {
-        queuedRequest.abort()
-        socket.send('cancel', {})
-        this.resetUploaderReferences(file.id)
+      this.onCancelAll(file.id, ({ reason } = {}) => {
+        if (reason === 'user') {
+          queuedRequest.abort()
+          socket.send('cancel', {})
+          this.resetUploaderReferences(file.id)
+        }
         resolve(`upload ${file.id} was canceled`)
       })
 
@@ -668,12 +672,12 @@ module.exports = class Tus extends BasePlugin {
 
   /**
    * @param {string} fileID
-   * @param {function(): void} cb
+   * @param {function(): void} eventHandler
    */
-  onCancelAll (fileID, cb) {
-    this.uploaderEvents[fileID].on('cancel-all', () => {
+  onCancelAll (fileID, eventHandler) {
+    this.uploaderEvents[fileID].on('cancel-all', (...args) => {
       if (!this.uppy.getFile(fileID)) return
-      cb()
+      eventHandler(...args)
     })
   }
 

diff --git a/packages/@uppy/xhr-upload/src/index.js b/packages/@uppy/xhr-upload/src/index.js
@@ -343,8 +343,10 @@ module.exports = class XHRUpload extends BasePlugin {
         reject(new Error('File removed'))
       })
 
-      this.onCancelAll(file.id, () => {
-        queuedRequest.abort()
+      this.onCancelAll(file.id, ({ reason }) => {
+        if (reason === 'user') {
+          queuedRequest.abort()
+        }
         reject(new Error('Upload cancelled'))
       })
     })
@@ -388,9 +390,11 @@ module.exports = class XHRUpload extends BasePlugin {
           resolve(`upload ${file.id} was removed`)
         })
 
-        this.onCancelAll(file.id, () => {
-          socket.send('cancel', {})
-          queuedRequest.abort()
+        this.onCancelAll(file.id, ({ reason } = {}) => {
+          if (reason === 'user') {
+            socket.send('cancel', {})
+            queuedRequest.abort()
+          }
           resolve(`upload ${file.id} was canceled`)
         })
 
@@ -528,7 +532,8 @@ module.exports = class XHRUpload extends BasePlugin {
         return reject(error)
       })
 
-      this.uppy.on('cancel-all', () => {
+      this.uppy.on('cancel-all', ({ reason } = {}) => {
+        if (reason !== 'user') return
         timer.done()
         xhr.abort()
       })
@@ -590,10 +595,10 @@ module.exports = class XHRUpload extends BasePlugin {
     })
   }
 
-  onCancelAll (fileID, cb) {
-    this.uploaderEvents[fileID].on('cancel-all', () => {
+  onCancelAll (fileID, eventHandler) {
+    this.uploaderEvents[fileID].on('cancel-all', (...args) => {
       if (!this.uppy.getFile(fileID)) return
-      cb()
+      eventHandler(...args)
     })
   }
 

diff --git a/website/src/docs/core.md b/website/src/docs/core.md
@@ -559,10 +559,6 @@ Retry an upload (after an error, for example).
 
 Retry all uploads (after an error, for example).
 
-### `uppy.cancelAll()`
-
-Cancel all uploads, reset progress and remove all files.
-
 ### `uppy.setState(patch)`
 
 Update Uppy’s internal state. Usually, this method is called internally, but in some cases it might be useful to alter something directly, especially when implementing your own plugins.
@@ -673,14 +669,20 @@ uppy.getPlugin('Dashboard').setOptions({
 })
 ```
 
-### `uppy.reset()`
+### `uppy.reset({ reason = 'user' })` (alias `uppy.cancelAll()`)
 
 Stop all uploads in progress and clear file selection, set progress to 0. More or less, it returns things to the way they were before any user input.
 
-### `uppy.close()`
+* `reason` - The reason for resetting. Plugins can use this to provide different cleanup behavior. Possible values are:
+  * `user` - The user has closed the Uppy instance
+  * `unmount` - The uppy instance has been closed programatically
+
+### `uppy.close({ reason = 'user' })`
 
 Uninstall all plugins and close down this Uppy instance. Also runs `uppy.reset()` before uninstalling.
 
+* `reason` - Same as the `reason` option for `cancelAll`
+
 ### `uppy.logout()`
 
 Calls `provider.logout()` on each remote provider plugin (Google Drive, Instagram, etc). Useful, for example, after your users log out of their account in your app — this will clean things up with Uppy cloud providers as well, for extra security.

diff --git a/website/src/docs/react-dashboard-modal.md b/website/src/docs/react-dashboard-modal.md
@@ -88,7 +88,7 @@ class MusicUploadButton extends React.Component {
   }
 
   componentWillUnmount () {
-    this.uppy.close()
+    this.uppy.close({ reason: 'unmount' })
   }
 
   handleOpen () {

diff --git a/website/src/docs/react-dashboard.md b/website/src/docs/react-dashboard.md
@@ -59,8 +59,8 @@ function Uploader () {
       .use(Webcam, { id: 'MyWebcam' }) // `id` is… "MyWebcam"
   }, [])
   React.useEffect(() => {
-    return () => uppy.close()
-  }, [])
+    return () => uppy.close({ reason: 'unmount' })
+  }, [uppy])
 
   return (
     <Dashboard

diff --git a/website/src/docs/react-initializing.md b/website/src/docs/react-initializing.md
@@ -53,7 +53,7 @@ class MyComponent extends React.Component {
   }
 
   componentWillUnmount () {
-    this.uppy.close()
+    this.uppy.close({ reason: 'unmount' })
   }
 
   render () {

diff --git a/website/src/docs/transloadit.md b/website/src/docs/transloadit.md
@@ -412,3 +412,7 @@ uppy.on('transloadit:complete', (assembly) => {
 [assembly-status]: https://transloadit.com/docs/api/#assembly-status-response
 
 [template-credentials]: https://transloadit.com/docs/#how-to-create-template-credentials
+
+## Assembly behavior when Uppy is closed
+
+When integrating `@uppy/transloadit` with `@uppy/dashboard`, closing the dashboard will result in continuing assemblies on the server. When the user manually cancels the upload any running assemblies will be cancelled.