From 1463ee79cea25aa623b69cc8027683f7b8b5f3c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Mon, 13 Jan 2020 12:29:08 +0100 Subject: [PATCH] Improve performance of adding and removing files (#1949) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * core: add an addFiles() method that only updates state once Previously, adding 1300-ish files took around 260ms, and looked like this in Firefox's performance tab: ![Flamegraph](https://i.imgur.com/08veuoU.png) All of the downward peaks are `setState()` calls and GC. Now it takes around 60ms, and looks like this: ![Flamegraph](https://i.imgur.com/xFdwVBV.png) Here, most of the time is spent generating file IDs and guessing file types. Those would be areas to look at next. * dashboard: prevent frequent state update if nothing changed After the last commit, `addFiles()` still spends a lot of time in `emit()` and `hideAllPanels()`. The reason is that `addFiles()` still emits all the hundreds of file-added events, and the Dashboard responds to each with `hideAllPanels()`, which does a state update. But this all happens synchronously, and the state almost certainly did not change since the last `file-added` event that fired a millisecond ago. This adds a check to avoid the state update if it is not necessary. ![Flamegraph](https://i.imgur.com/KhuD035.png) Adding 1300 files takes about 40ms now. With this change, the `addFiles()` call is no longer the slowest part—now preact rendering all the items is! * utils: optimize generateFileID and getFileNameAndExtension Replaces some clever things with more mundane and faster things! Now, generateFileID is a bunch of string concatenations. Now, getFileNameAndExtension uses `lastIndexOf()` instead of a regex. ![Flamegraph](https://i.imgur.com/ZQ1IhxI.png) Adding 1300 files takes about 25ms. * dashboard: use preact-virtual-list * thumbnail-generator: add `lazy` option * dashboard: request thumbnails once file item renders the first time * dashboard: fork preact-virtual-list * core: add removeFiles() to remove files in bulk * Implement removeFile() in terms of removeFiles() * thumbnail-generator: only queue files that can be previewed * rename size constants to accommodate WIDTH/HEIGHT * Use new uppy.addFiles in DragDrop and FileInput * utils: fix getFileNameAndExtension() type * Rip out the lazy thumbnail generation * Rip out virtualization. * Remove virtualization leftovers * tell future people that this is intentionally verbose * Update package-lock.json * henlo i am spell * Make `addFiles()` respect maxNumberOfFiles * core: show an informer error if some files fail in bulk add * locales: fix quotes to make build:locale-pack happy Co-authored-by: Artur Paikin --- packages/@uppy/core/src/index.js | 215 +++++++++++++----- .../dashboard/src/components/Dashboard.js | 43 ++-- .../src/components/FileItem/index.js | 130 ++++++----- .../src/components/FileItem/index.scss | 4 +- .../dashboard/src/components/FileList.js | 36 +-- packages/@uppy/dashboard/src/index.js | 58 ++--- packages/@uppy/dashboard/src/utils/pure.js | 21 -- packages/@uppy/drag-drop/src/index.js | 42 ++-- packages/@uppy/file-input/src/index.js | 32 +-- packages/@uppy/locales/src/en_US.js | 4 + .../@uppy/thumbnail-generator/src/index.js | 6 +- packages/@uppy/utils/src/generateFileID.js | 34 ++- .../utils/src/getFileNameAndExtension.js | 18 +- packages/@uppy/utils/types/index.d.ts | 2 +- 14 files changed, 386 insertions(+), 259 deletions(-) delete mode 100644 packages/@uppy/dashboard/src/utils/pure.js diff --git a/packages/@uppy/core/src/index.js b/packages/@uppy/core/src/index.js index ab57efbfe9..f27bc16d13 100644 --- a/packages/@uppy/core/src/index.js +++ b/packages/@uppy/core/src/index.js @@ -35,6 +35,10 @@ class Uppy { constructor (opts) { this.defaultLocale = { strings: { + addBulkFilesFailed: { + 0: 'Failed to add %{smart_count} file due to an internal error', + 1: 'Failed to add %{smart_count} files due to internal errors' + }, youCanOnlyUploadX: { 0: 'You can only upload %{smart_count} file', 1: 'You can only upload %{smart_count} files', @@ -416,14 +420,15 @@ class Uppy { * Check if file passes a set of restrictions set in options: maxFileSize, * maxNumberOfFiles and allowedFileTypes. * + * @param {object} files Object of IDs → files already added * @param {object} file object to check * @private */ - _checkRestrictions (file) { + _checkRestrictions (files, file) { const { maxFileSize, maxNumberOfFiles, allowedFileTypes } = this.opts.restrictions if (maxNumberOfFiles) { - if (Object.keys(this.getState().files).length + 1 > maxNumberOfFiles) { + if (Object.keys(files).length + 1 > maxNumberOfFiles) { throw new RestrictionError(`${this.i18n('youCanOnlyUploadX', { smart_count: maxNumberOfFiles })}`) } } @@ -479,21 +484,22 @@ class Uppy { throw (typeof err === 'object' ? err : new Error(err)) } - /** - * Add a new file to `state.files`. This will run `onBeforeFileAdded`, - * try to guess file type in a clever way, check file against restrictions, - * and start an upload if `autoProceed === true`. - * - * @param {object} file object to add - * @returns {string} id for the added file - */ - addFile (file) { - const { files, allowNewUpload } = this.getState() + _assertNewUploadAllowed (file) { + const { allowNewUpload } = this.getState() if (allowNewUpload === false) { this._showOrLogErrorAndThrow(new RestrictionError('Cannot add new files: already uploading.'), { file }) } + } + /** + * Create a file state object based on user-provided `addFile()` options. + * + * Note this is extremely side-effectful and should only be done when a file state object will be added to state immediately afterward! + * + * The `files` value is passed in because it may be updated by the caller without updating the store. + */ + _checkAndCreateFileStateObject (files, file) { const fileType = getFileType(file) file.type = fileType @@ -536,7 +542,10 @@ class Uppy { id: fileID, name: fileName, extension: fileExtension || '', - meta: Object.assign({}, this.getState().meta, meta), + meta: { + ...this.getState().meta, + ...meta + }, type: fileType, data: file.data, progress: { @@ -553,20 +562,16 @@ class Uppy { } try { - this._checkRestrictions(newFile) + this._checkRestrictions(files, newFile) } catch (err) { this._showOrLogErrorAndThrow(err, { file: newFile }) } - this.setState({ - files: Object.assign({}, files, { - [fileID]: newFile - }) - }) - - this.emit('file-added', newFile) - this.log(`Added file: ${fileName}, ${fileID}, mime type: ${fileType}`) + return newFile + } + // Schedule an upload if `autoProceed` is enabled. + _startIfAutoProceed () { if (this.opts.autoProceed && !this.scheduledAutoProceed) { this.scheduledAutoProceed = setTimeout(() => { this.scheduledAutoProceed = null @@ -577,49 +582,153 @@ class Uppy { }) }, 4) } + } - return fileID + /** + * Add a new file to `state.files`. This will run `onBeforeFileAdded`, + * try to guess file type in a clever way, check file against restrictions, + * and start an upload if `autoProceed === true`. + * + * @param {object} file object to add + * @returns {string} id for the added file + */ + addFile (file) { + this._assertNewUploadAllowed(file) + + const { files } = this.getState() + const newFile = this._checkAndCreateFileStateObject(files, file) + + this.setState({ + files: { + ...files, + [newFile.id]: newFile + } + }) + + this.emit('file-added', newFile) + this.log(`Added file: ${newFile.name}, ${newFile.id}, mime type: ${newFile.type}`) + + this._startIfAutoProceed() + + return newFile.id } - removeFile (fileID) { + /** + * Add multiple files to `state.files`. See the `addFile()` documentation. + * + * This cuts some corners for performance, so should typically only be used in cases where there may be a lot of files. + * + * If an error occurs while adding a file, it is logged and the user is notified. This is good for UI plugins, but not for programmatic use. Programmatic users should usually still use `addFile()` on individual files. + */ + addFiles (fileDescriptors) { + this._assertNewUploadAllowed() + + // create a copy of the files object only once + const files = { ...this.getState().files } + const newFiles = [] + const errors = [] + for (let i = 0; i < fileDescriptors.length; i++) { + try { + const newFile = this._checkAndCreateFileStateObject(files, fileDescriptors[i]) + newFiles.push(newFile) + files[newFile.id] = newFile + } catch (err) { + if (!err.isRestriction) { + errors.push(err) + } + } + } + + this.setState({ files }) + + newFiles.forEach((newFile) => { + this.emit('file-added', newFile) + }) + this.log(`Added batch of ${newFiles.length} files`) + + this._startIfAutoProceed() + + if (errors.length > 0) { + let message = 'Multiple errors occurred while adding files:\n' + errors.forEach((subError) => { + message += `\n * ${subError.message}` + }) + + this.info({ + message: this.i18n('addBulkFilesFailed', { smart_count: errors.length }), + details: message + }, 'error', 5000) + + const err = new Error(message) + err.errors = errors + throw err + } + } + + removeFiles (fileIDs) { const { files, currentUploads } = this.getState() - const updatedFiles = Object.assign({}, files) - const removedFile = updatedFiles[fileID] - delete updatedFiles[fileID] + const updatedFiles = { ...files } + const updatedUploads = { ...currentUploads } + + const removedFiles = Object.create(null) + fileIDs.forEach((fileID) => { + if (files[fileID]) { + removedFiles[fileID] = files[fileID] + delete updatedFiles[fileID] + } + }) - // Remove this file from its `currentUpload`. - const updatedUploads = Object.assign({}, currentUploads) - const removeUploads = [] + // Remove files from the `fileIDs` list in each upload. + function fileIsNotRemoved (uploadFileID) { + return removedFiles[uploadFileID] === undefined + } + const uploadsToRemove = [] Object.keys(updatedUploads).forEach((uploadID) => { - const newFileIDs = currentUploads[uploadID].fileIDs.filter((uploadFileID) => uploadFileID !== fileID) + const newFileIDs = currentUploads[uploadID].fileIDs.filter(fileIsNotRemoved) + // Remove the upload if no files are associated with it anymore. if (newFileIDs.length === 0) { - removeUploads.push(uploadID) + uploadsToRemove.push(uploadID) return } - updatedUploads[uploadID] = Object.assign({}, currentUploads[uploadID], { + updatedUploads[uploadID] = { + ...currentUploads[uploadID], fileIDs: newFileIDs - }) + } }) - this.setState({ - currentUploads: updatedUploads, - files: updatedFiles, - ...( - // If this is the last file we just removed - allow new uploads! - Object.keys(updatedFiles).length === 0 && - { allowNewUpload: true } - ) + uploadsToRemove.forEach((uploadID) => { + delete updatedUploads[uploadID] }) - removeUploads.forEach((uploadID) => { - this._removeUpload(uploadID) - }) + const stateUpdate = { + currentUploads: updatedUploads, + files: updatedFiles + } + + // If all files were removed - allow new uploads! + if (Object.keys(updatedFiles).length === 0) { + stateUpdate.allowNewUpload = true + } + + this.setState(stateUpdate) this._calculateTotalProgress() - this.emit('file-removed', removedFile) - this.log(`File removed: ${removedFile.id}`) + + const removedFileIDs = Object.keys(removedFiles) + removedFileIDs.forEach((fileID) => { + this.emit('file-removed', removedFiles[fileID]) + }) + if (removedFileIDs.length > 5) { + this.log(`Removed ${removedFileIDs.length} files`) + } else { + this.log(`Removed files: ${removedFileIDs.join(', ')}`) + } + } + + removeFile (fileID) { + this.removeFiles([fileID]) } pauseResume (fileID) { @@ -704,10 +813,12 @@ class Uppy { cancelAll () { this.emit('cancel-all') - const files = Object.keys(this.getState().files) - files.forEach((fileID) => { - this.removeFile(fileID) - }) + const { files } = this.getState() + + const fileIDs = Object.keys(files) + if (fileIDs.length) { + this.removeFiles(fileIDs) + } this.setState({ totalProgress: 0, @@ -1231,7 +1342,7 @@ class Uppy { * @param {string} uploadID The ID of the upload. */ _removeUpload (uploadID) { - const currentUploads = Object.assign({}, this.getState().currentUploads) + const currentUploads = { ...this.getState().currentUploads } delete currentUploads[uploadID] this.setState({ diff --git a/packages/@uppy/dashboard/src/components/Dashboard.js b/packages/@uppy/dashboard/src/components/Dashboard.js index 862c89760b..f552cb309e 100644 --- a/packages/@uppy/dashboard/src/components/Dashboard.js +++ b/packages/@uppy/dashboard/src/components/Dashboard.js @@ -24,24 +24,31 @@ function TransitionWrapper (props) { ) } +const WIDTH_XL = 900 +const WIDTH_LG = 700 +const WIDTH_MD = 576 +const HEIGHT_MD = 576 + module.exports = function Dashboard (props) { const noFiles = props.totalFileCount === 0 - const dashboardClassName = classNames( - { 'uppy-Root': props.isTargetDOMEl }, - 'uppy-Dashboard', - { 'Uppy--isTouchDevice': isTouchDevice() }, - { 'uppy-Dashboard--animateOpenClose': props.animateOpenClose }, - { 'uppy-Dashboard--isClosing': props.isClosing }, - { 'uppy-Dashboard--isDraggingOver': props.isDraggingOver }, - { 'uppy-Dashboard--modal': !props.inline }, - { 'uppy-size--md': props.containerWidth > 576 }, - { 'uppy-size--lg': props.containerWidth > 700 }, - { 'uppy-size--xl': props.containerWidth > 900 }, - { 'uppy-size--height-md': props.containerHeight > 400 }, - { 'uppy-Dashboard--isAddFilesPanelVisible': props.showAddFilesPanel }, - { 'uppy-Dashboard--isInnerWrapVisible': props.areInsidesReadyToBeVisible } - ) + const dashboardClassName = classNames({ + 'uppy-Root': props.isTargetDOMEl, + 'uppy-Dashboard': true, + 'Uppy--isTouchDevice': isTouchDevice(), + 'uppy-Dashboard--animateOpenClose': props.animateOpenClose, + 'uppy-Dashboard--isClosing': props.isClosing, + 'uppy-Dashboard--isDraggingOver': props.isDraggingOver, + 'uppy-Dashboard--modal': !props.inline, + 'uppy-size--md': props.containerWidth > WIDTH_MD, + 'uppy-size--lg': props.containerWidth > WIDTH_LG, + 'uppy-size--xl': props.containerWidth > WIDTH_XL, + 'uppy-size--height-md': props.containerHeight > HEIGHT_MD, + 'uppy-Dashboard--isAddFilesPanelVisible': props.showAddFilesPanel, + 'uppy-Dashboard--isInnerWrapVisible': props.areInsidesReadyToBeVisible + }) + + const showFileList = props.showSelectedFiles && !noFiles return (
- {(!noFiles && props.showSelectedFiles) && } + {showFileList && } - {props.showSelectedFiles ? ( - noFiles ? : + {showFileList ? ( + ) : ( )} diff --git a/packages/@uppy/dashboard/src/components/FileItem/index.js b/packages/@uppy/dashboard/src/components/FileItem/index.js index fd093609a9..e99030a9ad 100644 --- a/packages/@uppy/dashboard/src/components/FileItem/index.js +++ b/packages/@uppy/dashboard/src/components/FileItem/index.js @@ -1,76 +1,82 @@ -const { h } = require('preact') +const { h, Component } = require('preact') const classNames = require('classnames') -const pure = require('../../utils/pure') +const shallowEqual = require('is-shallow-equal') const FilePreviewAndLink = require('./FilePreviewAndLink') const FileProgress = require('./FileProgress') const FileInfo = require('./FileInfo') const Buttons = require('./Buttons') -module.exports = pure(function FileItem (props) { - const file = props.file +module.exports = class FileItem extends Component { + shouldComponentUpdate (nextProps) { + return !shallowEqual(this.props, nextProps) + } - const isProcessing = file.progress.preprocess || file.progress.postprocess - const isUploaded = file.progress.uploadComplete && !isProcessing && !file.error - const uploadInProgressOrComplete = file.progress.uploadStarted || isProcessing - const uploadInProgress = (file.progress.uploadStarted && !file.progress.uploadComplete) || isProcessing - const isPaused = file.isPaused || false - const error = file.error || false + render () { + const file = this.props.file - const showRemoveButton = props.individualCancellation - ? !isUploaded - : !uploadInProgress && !isUploaded + const isProcessing = file.progress.preprocess || file.progress.postprocess + const isUploaded = file.progress.uploadComplete && !isProcessing && !file.error + const uploadInProgressOrComplete = file.progress.uploadStarted || isProcessing + const uploadInProgress = (file.progress.uploadStarted && !file.progress.uploadComplete) || isProcessing + const isPaused = file.isPaused || false + const error = file.error || false - const dashboardItemClass = classNames( - 'uppy-u-reset', - 'uppy-DashboardItem', - { 'is-inprogress': uploadInProgress }, - { 'is-processing': isProcessing }, - { 'is-complete': isUploaded }, - { 'is-paused': isPaused }, - { 'is-error': !!error }, - { 'is-resumable': props.resumableUploads }, - { 'is-noIndividualCancellation': !props.individualCancellation } - ) + const showRemoveButton = this.props.individualCancellation + ? !isUploaded + : !uploadInProgress && !isUploaded - return ( -
  • -
    - - -
    + const dashboardItemClass = classNames({ + 'uppy-u-reset': true, + 'uppy-DashboardItem': true, + 'is-inprogress': uploadInProgress, + 'is-processing': isProcessing, + 'is-complete': isUploaded, + 'is-paused': isPaused, + 'is-error': !!error, + 'is-resumable': this.props.resumableUploads, + 'is-noIndividualCancellation': !this.props.individualCancellation + }) -
    - - +
    + + +
    - showLinkToFileUploadResult={props.showLinkToFileUploadResult} - showRemoveButton={showRemoveButton} +
    + + -
    -
    • - ) -}) + uploadInProgressOrComplete={uploadInProgressOrComplete} + removeFile={this.props.removeFile} + toggleFileCard={this.props.toggleFileCard} + + i18n={this.props.i18n} + log={this.props.log} + info={this.props.info} + /> +
    + + ) + } +} diff --git a/packages/@uppy/dashboard/src/components/FileItem/index.scss b/packages/@uppy/dashboard/src/components/FileItem/index.scss index e97b496d01..43223614a5 100644 --- a/packages/@uppy/dashboard/src/components/FileItem/index.scss +++ b/packages/@uppy/dashboard/src/components/FileItem/index.scss @@ -17,9 +17,7 @@ .uppy-size--md & { // For the Remove button position: relative; - - display: block; - float: left; + display: inline-block; margin: 5px $rl-margin; width: calc(33.333% - #{$rl-margin} - #{$rl-margin}); height: 215px; diff --git a/packages/@uppy/dashboard/src/components/FileList.js b/packages/@uppy/dashboard/src/components/FileList.js index 2ad70a6349..29a9831ad0 100644 --- a/packages/@uppy/dashboard/src/components/FileList.js +++ b/packages/@uppy/dashboard/src/components/FileList.js @@ -3,11 +3,10 @@ const classNames = require('classnames') const { h } = require('preact') module.exports = (props) => { - const noFiles = props.totalFileCount === 0 - const dashboardFilesClass = classNames( - 'uppy-Dashboard-files', - { 'uppy-Dashboard-files--noFiles': noFiles } - ) + const dashboardFilesClass = classNames({ + 'uppy-Dashboard-files': true, + 'uppy-Dashboard-files--noFiles': props.totalFileCount === 0 + }) const fileProps = { // FIXME This is confusing, it's actually the Dashboard's plugin ID @@ -32,22 +31,23 @@ module.exports = (props) => { pauseUpload: props.pauseUpload, cancelUpload: props.cancelUpload, toggleFileCard: props.toggleFileCard, - removeFile: props.removeFile + removeFile: props.removeFile, + handleRequestThumbnail: props.handleRequestThumbnail + } + + function renderItem (fileID) { + return ( + + ) } return ( -
      not focusable for firefox - tabindex="-1" - > - {Object.keys(props.files).map((fileID) => ( - - ))} +
        + {Object.keys(props.files).map(renderItem)}
      ) } diff --git a/packages/@uppy/dashboard/src/index.js b/packages/@uppy/dashboard/src/index.js index b690f78099..86f3bce416 100644 --- a/packages/@uppy/dashboard/src/index.js +++ b/packages/@uppy/dashboard/src/index.js @@ -220,11 +220,21 @@ module.exports = class Dashboard extends Plugin { } hideAllPanels () { - this.setPluginState({ + const update = { activePickerPanel: false, showAddFilesPanel: false, activeOverlayType: null - }) + } + + const current = this.getPluginState() + if (current.activePickerPanel === update.activePickerPanel && + current.showAddFilesPanel === update.showAddFilesPanel && + current.activeOverlayType === update.activeOverlayType) { + // avoid doing a state update if nothing changed + return + } + + this.setPluginState(update) } showPanel (id) { @@ -373,23 +383,23 @@ module.exports = class Dashboard extends Plugin { }) } - addFile (file) { + addFiles (files) { + const descriptors = files.map((file) => ({ + source: this.id, + name: file.name, + type: file.type, + data: file, + meta: { + // path of the file relative to the ancestor directory the user selected. + // e.g. 'docs/Old Prague/airbnb.pdf' + relativePath: file.relativePath || null + } + })) + try { - this.uppy.addFile({ - source: this.id, - name: file.name, - type: file.type, - data: file, - meta: { - // path of the file relative to the ancestor directory the user selected. - // e.g. 'docs/Old Prague/airbnb.pdf' - relativePath: file.relativePath || null - } - }) + this.uppy.addFiles(descriptors) } catch (err) { - if (!err.isRestriction) { - this.uppy.log(err) - } + this.uppy.log(err) } } @@ -504,18 +514,13 @@ module.exports = class Dashboard extends Plugin { // 2. Add all dropped files const files = toArray(event.clipboardData.files) - files.forEach((file) => { - this.uppy.log('[Dashboard] File pasted') - this.addFile(file) - }) + this.addFiles(files) } handleInputChange (event) { event.preventDefault() const files = toArray(event.target.files) - files.forEach((file) => - this.addFile(file) - ) + this.addFiles(files) } handleDragOver (event) { @@ -572,9 +577,7 @@ module.exports = class Dashboard extends Plugin { .then((files) => { if (files.length > 0) { this.uppy.log('[Dashboard] Files were dropped') - files.forEach((file) => - this.addFile(file) - ) + this.addFiles(files) } }) } @@ -817,7 +820,6 @@ module.exports = class Dashboard extends Plugin { log: this.uppy.log, i18n: this.i18n, i18nArray: this.i18nArray, - addFile: this.uppy.addFile, removeFile: this.uppy.removeFile, info: this.uppy.info, note: this.opts.note, diff --git a/packages/@uppy/dashboard/src/utils/pure.js b/packages/@uppy/dashboard/src/utils/pure.js deleted file mode 100644 index 577f507f53..0000000000 --- a/packages/@uppy/dashboard/src/utils/pure.js +++ /dev/null @@ -1,21 +0,0 @@ -const shallowEqual = require('is-shallow-equal') -const { h, Component } = require('preact') - -/** - * Higher order component that doesn't rerender an element if its props didn't change. - */ -module.exports = function pure (Inner) { - return class Pure extends Component { - shouldComponentUpdate (nextProps) { - return !shallowEqual(this.props, nextProps) - } - - render () { - // we have to clone this or Preact mutates it: - // https://github.com/preactjs/preact/issues/836 - // TODO can be removed if we upgrade to Preact X - const props = { ...this.props } - return - } - } -} diff --git a/packages/@uppy/drag-drop/src/index.js b/packages/@uppy/drag-drop/src/index.js index 205ac9b741..fbebf1de36 100644 --- a/packages/@uppy/drag-drop/src/index.js +++ b/packages/@uppy/drag-drop/src/index.js @@ -44,11 +44,11 @@ module.exports = class DragDrop extends Plugin { this.i18nInit() // Bind `this` to class methods - this.handleInputChange = this.handleInputChange.bind(this) + this.onInputChange = this.onInputChange.bind(this) this.handleDragOver = this.handleDragOver.bind(this) this.handleDragLeave = this.handleDragLeave.bind(this) this.handleDrop = this.handleDrop.bind(this) - this.addFile = this.addFile.bind(this) + this.addFiles = this.addFiles.bind(this) this.render = this.render.bind(this) } @@ -64,28 +64,30 @@ module.exports = class DragDrop extends Plugin { this.setPluginState() // so that UI re-renders and we see the updated locale } - addFile (file) { + addFiles (files) { + const descriptors = files.map((file) => ({ + source: this.id, + name: file.name, + type: file.type, + data: file, + meta: { + // path of the file relative to the ancestor directory the user selected. + // e.g. 'docs/Old Prague/airbnb.pdf' + relativePath: file.relativePath || null + } + })) + try { - this.uppy.addFile({ - source: this.id, - name: file.name, - type: file.type, - data: file, - meta: { - relativePath: file.relativePath || null - } - }) + this.uppy.addFiles(descriptors) } catch (err) { - if (!err.isRestriction) { - this.uppy.log(err) - } + this.uppy.log(err) } } - handleInputChange (event) { + onInputChange (event) { this.uppy.log('[DragDrop] Files selected through input') const files = toArray(event.target.files) - files.forEach(this.addFile) + this.addFiles(files) // We clear the input after a file is selected, because otherwise // change event is not fired in Chrome and Safari when a file @@ -110,9 +112,7 @@ module.exports = class DragDrop extends Plugin { this.uppy.log(error, 'error') } getDroppedFiles(event.dataTransfer, { logDropError }) - .then((files) => { - files.forEach(this.addFile) - }) + .then((files) => this.addFiles(files)) } handleDragOver (event) { @@ -150,7 +150,7 @@ module.exports = class DragDrop extends Plugin { name={this.opts.inputName} multiple={restrictions.maxNumberOfFiles !== 1} accept={restrictions.allowedFileTypes} - onchange={this.handleInputChange} + onchange={this.onInputChange} /> ) } diff --git a/packages/@uppy/file-input/src/index.js b/packages/@uppy/file-input/src/index.js index 9ee0bf9078..107d55a5a3 100644 --- a/packages/@uppy/file-input/src/index.js +++ b/packages/@uppy/file-input/src/index.js @@ -50,25 +50,25 @@ module.exports = class FileInput extends Plugin { this.setPluginState() // so that UI re-renders and we see the updated locale } + addFiles (files) { + const descriptors = files.map((file) => ({ + source: this.id, + name: file.name, + type: file.type, + data: file + })) + + try { + this.uppy.addFiles(descriptors) + } catch (err) { + this.uppy.log(err) + } + } + handleInputChange (event) { this.uppy.log('[FileInput] Something selected through input...') - const files = toArray(event.target.files) - - files.forEach((file) => { - try { - this.uppy.addFile({ - source: this.id, - name: file.name, - type: file.type, - data: file - }) - } catch (err) { - if (!err.isRestriction) { - this.uppy.log(err) - } - } - }) + this.addFiles(files) // We clear the input after a file is selected, because otherwise // change event is not fired in Chrome and Safari when a file diff --git a/packages/@uppy/locales/src/en_US.js b/packages/@uppy/locales/src/en_US.js index 433429a8cf..8478e807e0 100644 --- a/packages/@uppy/locales/src/en_US.js +++ b/packages/@uppy/locales/src/en_US.js @@ -1,6 +1,10 @@ const en_US = {} en_US.strings = { + addBulkFilesFailed: { + '0': 'Failed to add %{smart_count} file due to an internal error', + '1': 'Failed to add %{smart_count} files due to internal errors' + }, addMore: 'Add more', addMoreFiles: 'Add more files', addingMoreFiles: 'Adding more files', diff --git a/packages/@uppy/thumbnail-generator/src/index.js b/packages/@uppy/thumbnail-generator/src/index.js index c1fe0ff6ab..861e0eaf69 100644 --- a/packages/@uppy/thumbnail-generator/src/index.js +++ b/packages/@uppy/thumbnail-generator/src/index.js @@ -302,7 +302,7 @@ module.exports = class ThumbnailGenerator extends Plugin { } onFileAdded = (file) => { - if (!file.preview) { + if (!file.preview && isPreviewSupported(file.type) && !file.isRemote) { this.addToQueue(file) } } @@ -362,8 +362,8 @@ module.exports = class ThumbnailGenerator extends Plugin { } install () { - this.uppy.on('file-added', this.onFileAdded) this.uppy.on('file-removed', this.onFileRemoved) + this.uppy.on('file-added', this.onFileAdded) this.uppy.on('restored', this.onRestored) if (this.opts.waitForThumbnailsBeforeUpload) { @@ -372,8 +372,8 @@ module.exports = class ThumbnailGenerator extends Plugin { } uninstall () { - this.uppy.off('file-added', this.onFileAdded) this.uppy.off('file-removed', this.onFileRemoved) + this.uppy.off('file-added', this.onFileAdded) this.uppy.off('restored', this.onRestored) if (this.opts.waitForThumbnailsBeforeUpload) { diff --git a/packages/@uppy/utils/src/generateFileID.js b/packages/@uppy/utils/src/generateFileID.js index 6f9a64c0d8..8f6edd0934 100644 --- a/packages/@uppy/utils/src/generateFileID.js +++ b/packages/@uppy/utils/src/generateFileID.js @@ -4,18 +4,32 @@ * * @param {object} file * @returns {string} the fileID - * */ module.exports = function generateFileID (file) { - // filter is needed to not join empty values with `-` - return [ - 'uppy', - file.name ? encodeFilename(file.name.toLowerCase()) : '', - file.type, - file.meta && file.meta.relativePath ? encodeFilename(file.meta.relativePath.toLowerCase()) : '', - file.data.size, - file.data.lastModified - ].filter(val => val).join('-') + // It's tempting to do `[items].filter(Boolean).join('-')` here, but that + // is slower! simple string concatenation is fast + + let id = 'uppy' + if (typeof file.name === 'string') { + id += '-' + encodeFilename(file.name.toLowerCase()) + } + + if (file.type !== undefined) { + id += '-' + file.type + } + + if (file.meta && typeof file.meta.relativePath === 'string') { + id += '-' + encodeFilename(file.meta.relativePath.toLowerCase()) + } + + if (file.data.size !== undefined) { + id += '-' + file.data.size + } + if (file.data.lastModified !== undefined) { + id += '-' + file.data.lastModified + } + + return id } function encodeFilename (name) { diff --git a/packages/@uppy/utils/src/getFileNameAndExtension.js b/packages/@uppy/utils/src/getFileNameAndExtension.js index 66d9196da4..f5c08b2e3c 100644 --- a/packages/@uppy/utils/src/getFileNameAndExtension.js +++ b/packages/@uppy/utils/src/getFileNameAndExtension.js @@ -5,11 +5,17 @@ * @returns {object} {name, extension} */ module.exports = function getFileNameAndExtension (fullFileName) { - var re = /(?:\.([^.]+))?$/ - var fileExt = re.exec(fullFileName)[1] - var fileName = fullFileName.replace('.' + fileExt, '') - return { - name: fileName, - extension: fileExt + const lastDot = fullFileName.lastIndexOf('.') + // these count as no extension: "no-dot", "trailing-dot." + if (lastDot === -1 || lastDot === fullFileName.length - 1) { + return { + name: fullFileName, + extension: undefined + } + } else { + return { + name: fullFileName.slice(0, lastDot), + extension: fullFileName.slice(lastDot + 1) + } } } diff --git a/packages/@uppy/utils/types/index.d.ts b/packages/@uppy/utils/types/index.d.ts index 71c2fc1ddc..5d65d8a520 100644 --- a/packages/@uppy/utils/types/index.d.ts +++ b/packages/@uppy/utils/types/index.d.ts @@ -119,7 +119,7 @@ declare module '@uppy/utils/lib/getETA' { } declare module '@uppy/utils/lib/getFileNameAndExtension' { - function getFileNameAndExtension(filename: string): { name: string, extension: string }; + function getFileNameAndExtension(filename: string): { name: string, extension: string | undefined }; export = getFileNameAndExtension }