Merge pull request #746 from transloadit/improvement/onBefore

Return false — abort adding file or upload

src/core/Core.js

@@ -289,9 +289,9 @@ class Uppy {
   *
   * @private
   */
-  _checkMinNumberOfFiles () {
+  _checkMinNumberOfFiles (files) {
     const {minNumberOfFiles} = this.opts.restrictions
-    if (Object.keys(this.getState().files).length < minNumberOfFiles) {
+    if (Object.keys(files).length < minNumberOfFiles) {
       throw new Error(`${this.i18n('youHaveToAtLeastSelectX', { smart_count: minNumberOfFiles })}`)
     }
   }
@@ -348,18 +348,19 @@ class Uppy {
       throw err
     }
 
-    let mappedFile
-    try {
-      mappedFile = this.opts.onBeforeFileAdded(file, files)
-    } catch (err) {
-      onError(err)
+    const onBeforeFileAddedResult = this.opts.onBeforeFileAdded(file, files)
+
+    if (onBeforeFileAddedResult === false) {
+      this.log('Not adding file because onBeforeFileAdded returned false')
+      return
     }
 
-    if (typeof mappedFile === 'object' && mappedFile) {
-      if (mappedFile.then) {
+    if (typeof onBeforeFileAddedResult === 'object' && onBeforeFileAddedResult) {
+      // warning after the change in 0.24
+      if (onBeforeFileAddedResult.then) {
         throw new TypeError('onBeforeFileAdded() returned a Promise, but this is no longer supported. It must be synchronous.')
       }
-      file = mappedFile
+      file = onBeforeFileAddedResult
     }
 
     const fileType = Utils.getFileType(file)
@@ -1136,18 +1137,31 @@ class Uppy {
       this.log('No uploader type plugins are used', 'warning')
     }
 
+    let files = this.getState().files
+    const onBeforeUploadResult = this.opts.onBeforeUpload(files)
+
+    if (onBeforeUploadResult === false) {
+      return Promise.reject(new Error('Not starting the upload because onBeforeUpload returned false'))
+    }
+
+    if (onBeforeUploadResult && typeof onBeforeUploadResult === 'object') {
+      // warning after the change in 0.24
+      if (onBeforeUploadResult.then) {
+        throw new TypeError('onBeforeUpload() returned a Promise, but this is no longer supported. It must be synchronous.')
+      }
+
+      files = onBeforeUploadResult
+    }
+
     return Promise.resolve()
-      .then(() => {
-        this.opts.onBeforeUpload(this.getState().files)
-      })
-      .then(() => this._checkMinNumberOfFiles())
+      .then(() => this._checkMinNumberOfFiles(files))
       .then(() => {
         const { currentUploads } = this.getState()
         // get a list of files that are currently assigned to uploads
         const currentlyUploadingFiles = Object.keys(currentUploads).reduce((prev, curr) => prev.concat(currentUploads[curr].fileIDs), [])
 
         const waitingFileIDs = []
-        Object.keys(this.getState().files).forEach((fileID) => {
+        Object.keys(files).forEach((fileID) => {
           const file = this.getFile(fileID)
           // if the file hasn't started uploading and hasn't already been assigned to an upload..
           if ((!file.progress.uploadStarted) && (currentlyUploadingFiles.indexOf(fileID) === -1)) {

src/core/Core.test.js

@@ -618,24 +618,21 @@ describe('src/Core', () => {
       }
     })
 
-    it('should work with restriction errors that are not Error class instances', () => {
+    it('should not allow a file if onBeforeFileAdded returned false', () => {
       const core = new Core({
-        onBeforeFileAdded () {
-          throw 'a plain string' // eslint-disable-line no-throw-literal
+        onBeforeFileAdded: (file, files) => {
+          if (file.source === 'jest') {
+            return false
+          }
         }
       })
-
-      try {
-        core.addFile({
-          source: 'jest',
-          name: 'foo.jpg',
-          type: 'image/jpeg',
-          data: null
-        })
-        throw new Error('should have thrown')
-      } catch (err) {
-        expect(err).toMatchObject(new Error('a plain string'))
-      }
+      core.addFile({
+        source: 'jest',
+        name: 'foo.jpg',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' })
+      })
+      expect(Object.keys(core.state.files).length).toEqual(0)
     })
   })
 
@@ -699,6 +696,40 @@ describe('src/Core', () => {
 
       return expect(core.upload()).resolves.toMatchSnapshot()
     })
+
+    it('should not upload if onBeforeUpload returned false', () => {
+      const core = new Core({
+        autoProceed: false,
+        onBeforeUpload: (files) => {
+          for (var fileId in files) {
+            if (files[fileId].name === '123.foo') {
+              return false
+            }
+          }
+        }
+      })
+      core.addFile({
+        source: 'jest',
+        name: 'foo.jpg',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' })
+      })
+      core.addFile({
+        source: 'jest',
+        name: 'bar.jpg',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' })
+      })
+      core.addFile({
+        source: 'jest',
+        name: '123.foo',
+        type: 'image/jpeg',
+        data: new File([sampleImage], { type: 'image/jpeg' })
+      })
+      return core.upload().catch((err) => {
+        expect(err).toMatchObject(new Error('Not starting the upload because onBeforeUpload returned false'))
+      })
+    })
   })
 
   describe('removing a file', () => {

src/plugins/Form.js

@@ -53,7 +53,9 @@ module.exports = class Form extends Plugin {
   handleFormSubmit (ev) {
     if (this.opts.triggerUploadOnSubmit) {
       ev.preventDefault()
-      this.uppy.upload()
+      this.uppy.upload().catch((err) => {
+        this.uppy.log(err.stack || err.message || err)
+      })
     }
   }
 

src/plugins/StatusBar/index.js

@@ -94,7 +94,8 @@ module.exports = class StatusBar extends Plugin {
   }
 
   startUpload () {
-    return this.uppy.upload().catch(() => {
+    return this.uppy.upload().catch((err) => {
+      this.uppy.log(err.stack || err.message || err)
       // Ignore
     })
   }

website/src/docs/uppy.md

@@ -79,29 +79,83 @@ Metadata can also be added from a `<form>` element on your page via [Form](/docs
 <a id="onBeforeFileAdded"></a>
 ### `onBeforeFileAdded: (currentFile, files) => currentFile`
 
-A function run before a file is added to Uppy. Gets passed `(currentFile, files)` where `currentFile` is a file that is about to be added, and `files` is an object with all files that already are in Uppy. Return a file object or nothing to proceed with adding the file, or throw an error to abort. Use this function to run any number of custom checks on the selected file, or manipulating it, like optimizing a file name, for example.
+A function run before a file is added to Uppy. Gets passed `(currentFile, files)` where `currentFile` is a file that is about to be added, and `files` is an object with all files that already are in Uppy. 
+
+Use this function to run any number of custom checks on the selected file, or manipulating it, like optimizing a file name, for example.
+
+Return true/nothing or a modified file object to proceed with adding the file:
 
 ```js
 onBeforeFileAdded: (currentFile, files) => {
   if (currentFile.name === 'forest-IMG_0616.jpg') {
     return true
   }
-  throw new Error('This is not the file I was looking for')
+}
+
+// or
+
+onBeforeFileAdded: (currentFile, files) => {
+  const modifiedFile = Object.assign(
+    {}, 
+    currentFile, 
+    { name: currentFile + Date.now()
+  })
+  return modifiedFile
+}
+```
+
+Return false to abort adding the file:
+
+```js
+onBeforeFileAdded: (currentFile, files) => {
+  if (!currentFile.type) {
+    // log to console
+    uppy.log(`Skipping file because it has no type`)
+    // show error message to the user
+    uppy.info(`Skipping file because it has no type`, 'error', 500)
+    return false
+  }
 }
 ```
 
-### `onBeforeUpload: (files) => {}`
+Note: it’s up to you to show a notification to the user about file not passing validation. We recommend showing the info message and logging to console for debugging.
+
+
+<a id="onBeforeUpload"></a>
+### `onBeforeUpload: (files) => files`
+
+A function run before an upload begins. Gets passed `files` object with all the files that are already in Uppy. 
+
+Use this to check if all files or their total number match your requirements, or manipulate all the files at once before upload.
+
+Return true or modified `files` object to proceed:
 
-A function run before an upload begins. Gets passed `files` object with all files that already are in Uppy. Return nothing to proceed with adding the file or throw an error to abort. Use this to check if all files or their total number match your requirements, or manipulate all the files at once before upload.
+```js
+onBeforeUpload: (files) => {
+  const updatedFiles = Object.assign({}, files)
+  Object.keys(updatedFiles).forEach(fileId => {
+    updatedFiles[fileId].name = 'myCustomPrefix_' + updatedFiles[fileId].name
+  })
+  return updatedFiles
+}
+```
+
+Return false to abort:
 
 ```js
 onBeforeUpload: (files) => {
   if (Object.keys(files).length < 2) {
-    throw new Error('Not enough files.')
+    // log to console
+    uppy.log(`Aborting upload because only ${Object.keys(files).length} files were selected`)
+    // show error message to the user
+    uppy.info(`You have to select at least 2 files`, 'error', 500)
+    return false
   }
 }
 ```
 
+Note: it’s up to you to show a notification to the user about file not passing validation. We recommend showing the info message and logging to console for debugging.
+
 ### `locale: {}`
 
 Same deal as in plugins, this allows you to override language strings: