From 5537bf4038c468b91e18a32cf7d20142c5cd2c5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 5 Feb 2018 14:38:32 +0100 Subject: [PATCH 1/3] core: Consistent addFile rejections --- src/core/Core.js | 88 +++++++++++---------------- src/core/Core.test.js | 10 +-- src/plugins/Transloadit/index.test.js | 2 +- 3 files changed, 42 insertions(+), 58 deletions(-) diff --git a/src/core/Core.js b/src/core/Core.js index 57d34f099..6027f1bee 100644 --- a/src/core/Core.js +++ b/src/core/Core.js @@ -278,10 +278,8 @@ class Uppy { _checkMinNumberOfFiles () { const {minNumberOfFiles} = this.opts.restrictions if (Object.keys(this.getState().files).length < minNumberOfFiles) { - this.info(`${this.i18n('youHaveToAtLeastSelectX', { smart_count: minNumberOfFiles })}`, 'error', 5000) - return false + throw new Error(`${this.i18n('youHaveToAtLeastSelectX', { smart_count: minNumberOfFiles })}`) } - return true } /** @@ -289,7 +287,6 @@ class Uppy { * maxNumberOfFiles and allowedFileTypes. * * @param {object} file object to check - * @return {boolean} * @private */ _checkRestrictions (file) { @@ -297,8 +294,7 @@ class Uppy { if (maxNumberOfFiles) { if (Object.keys(this.getState().files).length + 1 > maxNumberOfFiles) { - this.info(`${this.i18n('youCanOnlyUploadX', { smart_count: maxNumberOfFiles })}`, 'error', 5000) - return false + throw new Error(`${this.i18n('youCanOnlyUploadX', { smart_count: maxNumberOfFiles })}`) } } @@ -310,19 +306,15 @@ class Uppy { if (!isCorrectFileType) { const allowedFileTypesString = allowedFileTypes.join(', ') - this.info(`${this.i18n('youCanOnlyUploadFileTypes')} ${allowedFileTypesString}`, 'error', 5000) - return false + throw new Error(`${this.i18n('youCanOnlyUploadFileTypes')} ${allowedFileTypesString}`) } } if (maxFileSize) { if (file.data.size > maxFileSize) { - this.info(`${this.i18n('exceedsSize')} ${prettyBytes(maxFileSize)}`, 'error', 5000) - return false + throw new Error(`${this.i18n('exceedsSize')} ${prettyBytes(maxFileSize)}`) } } - - return true } /** @@ -333,17 +325,12 @@ class Uppy { * @param {object} file object to add */ addFile (file) { - // Wrap this in a Promise `.then()` handler so errors will reject the Promise - // instead of throwing. - const beforeFileAdded = Promise.resolve() + return Promise.resolve() + // Wrap this in a Promise `.then()` handler so errors will reject the Promise + // instead of throwing. .then(() => this.opts.onBeforeFileAdded(file, this.getState().files)) - - return beforeFileAdded.catch((err) => { - const message = typeof err === 'object' ? err.message : err - this.info(message, 'error', 5000) - return Promise.reject(new Error(`onBeforeFileAdded: ${message}`)) - }).then(() => { - return Utils.getFileType(file).then((fileType) => { + .then(() => Utils.getFileType(file)) + .then((fileType) => { const updatedFiles = Object.assign({}, this.getState().files) let fileName if (file.name) { @@ -382,10 +369,7 @@ class Uppy { preview: file.preview } - const isFileAllowed = this._checkRestrictions(newFile) - if (!isFileAllowed) { - return Promise.reject(new Error('File not allowed')) - } + this._checkRestrictions(newFile) updatedFiles[fileID] = newFile this.setState({files: updatedFiles}) @@ -402,7 +386,11 @@ class Uppy { }, 4) } }) - }) + .catch((err) => { + const message = typeof err === 'object' ? err.message : err + this.info(message, 'error', 5000) + return Promise.reject(typeof err === 'object' ? err : new Error(err)) + }) } removeFile (fileID) { @@ -1090,35 +1078,31 @@ class Uppy { this.log('No uploader type plugins are used', 'warning') } - const isMinNumberOfFilesReached = this._checkMinNumberOfFiles() - if (!isMinNumberOfFilesReached) { - return Promise.reject(new Error('Minimum number of files has not been reached')) - } - - const beforeUpload = Promise.resolve() + return Promise.resolve() .then(() => this.opts.onBeforeUpload(this.getState().files)) + .then(() => this._checkMinNumberOfFiles()) + .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), []) - return beforeUpload.catch((err) => { - const message = typeof err === 'object' ? err.message : err - this.info(message, 'error', 5000) - return Promise.reject(new Error(`onBeforeUpload: ${message}`)) - }).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) => { + 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)) { + waitingFileIDs.push(file.id) + } + }) - const waitingFileIDs = [] - Object.keys(this.getState().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)) { - waitingFileIDs.push(file.id) - } + const uploadID = this._createUpload(waitingFileIDs) + return this._runUpload(uploadID) + }) + .catch((err) => { + const message = typeof err === 'object' ? err.message : err + this.info(message, 'error', 5000) + return Promise.reject(typeof err === 'object' ? err : new Error(err)) }) - - const uploadID = this._createUpload(waitingFileIDs) - return this._runUpload(uploadID) - }) } } diff --git a/src/core/Core.test.js b/src/core/Core.test.js index 169bfe4f9..bfb534240 100644 --- a/src/core/Core.test.js +++ b/src/core/Core.test.js @@ -610,7 +610,7 @@ describe('src/Core', () => { throw new Error('File was allowed through') }) .catch(e => { - expect(e.message).toEqual('File not allowed') + expect(e.message).toEqual('You can only upload: image/gif') }) }) @@ -625,7 +625,7 @@ describe('src/Core', () => { name: 'foo.jpg', type: 'image/jpeg', data: null - })).rejects.toMatchObject(new Error('onBeforeFileAdded: a plain string')) + })).rejects.toMatchObject(new Error('a plain string')) }) }) @@ -955,7 +955,7 @@ describe('src/Core', () => { name: 'foo2.jpg', type: 'image/jpeg', data: utils.dataURItoFile(sampleImageDataURI, {}) - })).rejects.toMatchObject(new Error('File not allowed')).then(() => { + })).rejects.toMatchObject(new Error('You can only upload 1 file')).then(() => { expect(core.state.info.message).toEqual('You can only upload 1 file') }) }) @@ -975,7 +975,7 @@ describe('src/Core', () => { name: 'foo2.jpg', type: 'image/jpeg', data: utils.dataURItoFile(sampleImageDataURI, {}) - })).rejects.toMatchObject(new Error('File not allowed')).then(() => { + })).rejects.toMatchObject(new Error('You can only upload: image/gif, image/png')).then(() => { expect(core.state.info.message).toEqual('You can only upload: image/gif, image/png') }) }) @@ -993,7 +993,7 @@ describe('src/Core', () => { name: 'foo.jpg', type: 'image/jpeg', data: utils.dataURItoFile(sampleImageDataURI, {}) - })).rejects.toMatchObject(new Error('File not allowed')).then(() => { + })).rejects.toMatchObject(new Error('This file exceeds maximum allowed size of 1.2 KB')).then(() => { expect(core.state.info.message).toEqual('This file exceeds maximum allowed size of 1.2 KB') }) }) diff --git a/src/plugins/Transloadit/index.test.js b/src/plugins/Transloadit/index.test.js index 431bef298..269e75fc6 100644 --- a/src/plugins/Transloadit/index.test.js +++ b/src/plugins/Transloadit/index.test.js @@ -168,6 +168,6 @@ describe('Transloadit', () => { } }) - return expect(uppy.upload()).rejects.toBe('short-circuited') + return expect(uppy.upload()).rejects.toEqual(new Error('short-circuited')) }) }) From 26cafb1afe4e12d095fb5149e7d4cda7cd7c7e2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 5 Feb 2018 14:40:38 +0100 Subject: [PATCH 2/3] core: Prevent unhandled rejection warnings in addFile() --- src/core/Core.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/Core.js b/src/core/Core.js index 6027f1bee..537ac9a38 100644 --- a/src/core/Core.js +++ b/src/core/Core.js @@ -389,7 +389,13 @@ class Uppy { .catch((err) => { const message = typeof err === 'object' ? err.message : err this.info(message, 'error', 5000) - return Promise.reject(typeof err === 'object' ? err : new Error(err)) + const rejected = Promise.reject(typeof err === 'object' ? err : new Error(err)) + // Silence the unhandled rejection; we have already shown it to the user, + // so the consumer may not need to do anything with this result. + // Note that the result is still a rejected Promise, it will just not + // trigger unhandled rejection warnings in the browser console. + rejected.catch(() => {}) + return rejected }) } From 4b24537a08ec883afc6868daae7cdf2e338451ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 5 Feb 2018 14:58:59 +0100 Subject: [PATCH 3/3] Catch `addFile()` errors in UI plugins Prevents warnings in the console about unhandled rejections. We can safely catch and ignore these errors, because they also show up in the UI already anyway. --- src/core/Core.js | 8 +------- src/plugins/Dashboard/index.js | 6 ++++++ src/plugins/DragDrop/index.js | 4 ++++ src/plugins/Dummy.js | 4 +++- src/plugins/FileInput.js | 2 ++ src/plugins/Provider/view/index.js | 4 +++- src/plugins/Webcam/index.js | 6 ++++-- 7 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/core/Core.js b/src/core/Core.js index 537ac9a38..6027f1bee 100644 --- a/src/core/Core.js +++ b/src/core/Core.js @@ -389,13 +389,7 @@ class Uppy { .catch((err) => { const message = typeof err === 'object' ? err.message : err this.info(message, 'error', 5000) - const rejected = Promise.reject(typeof err === 'object' ? err : new Error(err)) - // Silence the unhandled rejection; we have already shown it to the user, - // so the consumer may not need to do anything with this result. - // Note that the result is still a rejected Promise, it will just not - // trigger unhandled rejection warnings in the browser console. - rejected.catch(() => {}) - return rejected + return Promise.reject(typeof err === 'object' ? err : new Error(err)) }) } diff --git a/src/plugins/Dashboard/index.js b/src/plugins/Dashboard/index.js index 760dce592..c4b84ae5f 100644 --- a/src/plugins/Dashboard/index.js +++ b/src/plugins/Dashboard/index.js @@ -277,6 +277,8 @@ module.exports = class Dashboard extends Plugin { name: file.name, type: file.type, data: blob + }).catch(() => { + // Ignore }) }) } @@ -291,6 +293,8 @@ module.exports = class Dashboard extends Plugin { name: file.name, type: file.type, data: file + }).catch(() => { + // Ignore }) }) } @@ -360,6 +364,8 @@ module.exports = class Dashboard extends Plugin { name: file.name, type: file.type, data: file + }).catch(() => { + // Ignore }) }) } diff --git a/src/plugins/DragDrop/index.js b/src/plugins/DragDrop/index.js index 8b6825e86..d74e97596 100644 --- a/src/plugins/DragDrop/index.js +++ b/src/plugins/DragDrop/index.js @@ -83,6 +83,8 @@ module.exports = class DragDrop extends Plugin { name: file.name, type: file.type, data: file + }).catch(() => { + // Ignore }) }) } @@ -98,6 +100,8 @@ module.exports = class DragDrop extends Plugin { name: file.name, type: file.type, data: file + }).catch(() => { + // Ignore }) }) } diff --git a/src/plugins/Dummy.js b/src/plugins/Dummy.js index 3a1e108fe..bd0d9bbc6 100644 --- a/src/plugins/Dummy.js +++ b/src/plugins/Dummy.js @@ -35,7 +35,9 @@ module.exports = class Dummy extends Plugin { data: blob } this.props.log('Adding fake file blob') - this.props.addFile(file) + this.props.addFile(file).catch(() => { + // Ignore + }) } render (state) { diff --git a/src/plugins/FileInput.js b/src/plugins/FileInput.js index 606cc9846..2ac5eaee3 100644 --- a/src/plugins/FileInput.js +++ b/src/plugins/FileInput.js @@ -51,6 +51,8 @@ module.exports = class FileInput extends Plugin { name: file.name, type: file.type, data: file + }).catch(() => { + // Ignore }) }) } diff --git a/src/plugins/Provider/view/index.js b/src/plugins/Provider/view/index.js index c99c728d4..92117558c 100644 --- a/src/plugins/Provider/view/index.js +++ b/src/plugins/Provider/view/index.js @@ -169,7 +169,9 @@ module.exports = class View { tagFile.preview = this.plugin.getItemThumbnailUrl(file) } this.plugin.uppy.log('Adding remote file') - this.plugin.uppy.addFile(tagFile) + this.plugin.uppy.addFile(tagFile).catch(() => { + // Ignore + }) if (!isCheckbox) { this.donePicking() } diff --git a/src/plugins/Webcam/index.js b/src/plugins/Webcam/index.js index 264223443..e7286b296 100644 --- a/src/plugins/Webcam/index.js +++ b/src/plugins/Webcam/index.js @@ -169,7 +169,7 @@ module.exports = class Webcam extends Plugin { }) return this.getVideo() }) - .then(this.uppy.addFile) + .then((file) => this.uppy.addFile(file)) .then(() => { this.recordingChunks = null this.recorder = null @@ -233,9 +233,11 @@ module.exports = class Webcam extends Plugin { return this.getImage() }).then((tagFile) => { this.captureInProgress = false - this.uppy.addFile(tagFile) const dashboard = this.uppy.getPlugin('Dashboard') if (dashboard) dashboard.hideAllPanels() + return this.uppy.addFile(tagFile).catch(() => { + // Ignore + }) }, (error) => { this.captureInProgress = false throw error