@uppy/core: improve performance of validating & uploading files

e2e/package.json

@@ -45,11 +45,11 @@
     "@uppy/zoom": "workspace:^"
   },
   "devDependencies": {
-    "cypress": "^10.0.0",
+    "cypress": "^12.9.0",
     "cypress-terminal-report": "^4.1.2",
     "deep-freeze": "^0.0.1",
     "execa": "^6.1.0",
-    "parcel": "^2.0.1",
+    "parcel": "2.0.0-nightly.1278",
     "prompts": "^2.4.2",
     "react": "^18.1.0",
     "react-dom": "^18.1.0",

package.json

@@ -45,7 +45,7 @@
     "@babel/preset-env": "^7.14.7",
     "@babel/register": "^7.10.5",
     "@babel/types": "^7.17.0",
-    "@parcel/transformer-vue": "2.7.0",
+    "@parcel/transformer-vue": "2.8.4-nightly.2903+5b901a317",
     "@types/jasmine": "file:./private/@types/jasmine",
     "@types/jasminewd2": "file:./private/@types/jasmine",
     "@typescript-eslint/eslint-plugin": "^5.0.0",

packages/@uppy/aws-s3-multipart/src/MultipartUploader.js

@@ -70,19 +70,29 @@ class MultipartUploader {
 
     // Upload zero-sized files in one zero-sized chunk
     if (this.#data.size === 0) {
-      this.#chunks = [this.#data]
-      this.#data.onProgress = this.#onPartProgress(0)
-      this.#data.onComplete = this.#onPartComplete(0)
+      this.#chunks = [{
+        getData: () => this.#data,
+        onProgress: this.#onPartProgress(0),
+        onComplete: this.#onPartComplete(0),
+      }]
     } else {
       const arraySize = Math.ceil(fileSize / chunkSize)
       this.#chunks = Array(arraySize)
-      let j = 0
-      for (let i = 0; i < fileSize; i += chunkSize) {
+
+      for (let i = 0, j = 0; i < fileSize; i += chunkSize, j++) {
         const end = Math.min(fileSize, i + chunkSize)
-        const chunk = this.#data.slice(i, end)
-        chunk.onProgress = this.#onPartProgress(j)
-        chunk.onComplete = this.#onPartComplete(j)
-        this.#chunks[j++] = chunk
+
+        // Defer data fetching/slicing until we actually need the data, because it's slow if we have a lot of files
+        const getData = () => {
+          const i2 = i
+          return this.#data.slice(i2, end)
+        }
+
+        this.#chunks[j] = {
+          getData,
+          onProgress: this.#onPartProgress(j),
+          onComplete: this.#onPartComplete(j),
+        }
       }
     }
 

packages/@uppy/aws-s3-multipart/src/index.js

@@ -4,7 +4,7 @@ import EventTracker from '@uppy/utils/lib/EventTracker'
 import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress'
 import getSocketHost from '@uppy/utils/lib/getSocketHost'
 import { RateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
-
+import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters'
 import { createAbortError } from '@uppy/utils/lib/AbortController'
 import packageJson from '../package.json'
 import MultipartUploader from './MultipartUploader.js'
@@ -200,17 +200,23 @@ class HTTPCommunicationQueue {
     return this.#sendCompletionRequest(file, { key, uploadId, parts, signal }).abortOn(signal)
   }
 
-  async uploadChunk (file, partNumber, body, signal) {
+  async uploadChunk (file, partNumber, chunk, signal) {
     throwIfAborted(signal)
     const { uploadId, key } = await this.getUploadId(file, signal)
     throwIfAborted(signal)
     for (;;) {
-      const signature = await this.#fetchSignature(file, { uploadId, key, partNumber, body, signal }).abortOn(signal)
+      const chunkData = chunk.getData()
+      const { onProgress, onComplete } = chunk
+
+      const signature = await this.#fetchSignature(file, {
+        uploadId, key, partNumber, body: chunkData, signal,
+      }).abortOn(signal)
+
       throwIfAborted(signal)
       try {
         return {
           PartNumber: partNumber,
-          ...await this.#uploadPartBytes(signature, body, signal).abortOn(signal),
+          ...await this.#uploadPartBytes({ signature, body: chunkData, onProgress, onComplete, signal }).abortOn(signal),
         }
       } catch (err) {
         if (!await this.#shouldRetry(err)) throw err
@@ -258,8 +264,6 @@ export default class AwsS3Multipart extends BasePlugin {
       }
     }
 
-    this.upload = this.upload.bind(this)
-
     /**
      * Simultaneous upload limiting is shared across all uploads with this plugin.
      *
@@ -369,7 +373,7 @@ export default class AwsS3Multipart extends BasePlugin {
       .then(assertServerError)
   }
 
-  static async uploadPartBytes ({ url, expires, headers }, body, signal) {
+  static async uploadPartBytes ({ signature: { url, expires, headers }, body, onProgress, onComplete, signal }) {
     throwIfAborted(signal)
 
     if (url == null) {
@@ -397,7 +401,7 @@ export default class AwsS3Multipart extends BasePlugin {
       }
       signal.addEventListener('abort', onabort)
 
-      xhr.upload.addEventListener('progress', body.onProgress)
+      xhr.upload.addEventListener('progress', onProgress)
 
       xhr.addEventListener('abort', () => {
         cleanup()
@@ -427,7 +431,7 @@ export default class AwsS3Multipart extends BasePlugin {
           return
         }
 
-        body.onProgress?.(body.size)
+        onProgress?.(body.size)
 
         // NOTE This must be allowed by CORS.
         const etag = ev.target.getResponseHeader('ETag')
@@ -437,7 +441,7 @@ export default class AwsS3Multipart extends BasePlugin {
           return
         }
 
-        body.onComplete?.(etag)
+        onComplete?.(etag)
         resolve({
           ETag: etag,
         })
@@ -466,8 +470,10 @@ export default class AwsS3Multipart extends BasePlugin {
     })
   }
 
-  uploadFile (file) {
+  #uploadFile (file) {
     return new Promise((resolve, reject) => {
+      const getFile = () => this.uppy.getFile(file.id) || file
+
       const onProgress = (bytesUploaded, bytesTotal) => {
         this.uppy.emit('upload-progress', file, {
           uploader: this,
@@ -485,7 +491,6 @@ export default class AwsS3Multipart extends BasePlugin {
       }
 
       const onSuccess = (result) => {
-        const uploadObject = upload // eslint-disable-line no-use-before-define
         const uploadResp = {
           body: {
             ...result,
@@ -495,23 +500,17 @@ export default class AwsS3Multipart extends BasePlugin {
 
         this.resetUploaderReferences(file.id)
 
-        const cFile = this.uppy.getFile(file.id)
-        this.uppy.emit('upload-success', cFile || file, uploadResp)
+        this.uppy.emit('upload-success', getFile(), uploadResp)
 
         if (result.location) {
           this.uppy.log(`Download ${file.name} from ${result.location}`)
         }
 
-        resolve(uploadObject)
+        resolve()
       }
 
       const onPartComplete = (part) => {
-        const cFile = this.uppy.getFile(file.id)
-        if (!cFile) {
-          return
-        }
-
-        this.uppy.emit('s3-multipart:part-uploaded', cFile, part)
+        this.uppy.emit('s3-multipart:part-uploaded', getFile(), part)
       }
 
       const upload = new MultipartUploader(file.data, {
@@ -564,11 +563,7 @@ export default class AwsS3Multipart extends BasePlugin {
         upload.start()
       })
 
-      // Don't double-emit upload-started for Golden Retriever-restored files that were already started
-      if (!file.progress.uploadStarted || !file.isRestored) {
-        upload.start()
-        this.uppy.emit('upload-started', file)
-      }
+      upload.start()
     })
   }
 
@@ -597,14 +592,9 @@ export default class AwsS3Multipart extends BasePlugin {
 
   // NOTE! Keep this duplicated code in sync with other plugins
   // TODO we should probably abstract this into a common function
-  async uploadRemote (file) {
+  async #uploadRemote (file) {
     this.resetUploaderReferences(file.id)
 
-    // Don't double-emit upload-started for Golden Retriever-restored files that were already started
-    if (!file.progress.uploadStarted || !file.isRestored) {
-      this.uppy.emit('upload-started', file)
-    }
-
     try {
       if (file.serverToken) {
         return await this.connectToServerSocket(file)
@@ -733,15 +723,20 @@ export default class AwsS3Multipart extends BasePlugin {
     })
   }
 
-  async upload (fileIDs) {
+  #upload = async (fileIDs) => {
     if (fileIDs.length === 0) return undefined
 
-    const promises = fileIDs.map((id) => {
-      const file = this.uppy.getFile(id)
+    const files = this.uppy.getFilesByIds(fileIDs)
+
+    const filesFiltered = filterNonFailedFiles(files)
+    const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
+    this.uppy.emit('upload-start', filesToEmit)
+
+    const promises = filesFiltered.map((file) => {
       if (file.isRemote) {
-        return this.uploadRemote(file)
+        return this.#uploadRemote(file)
       }
-      return this.uploadFile(file)
+      return this.#uploadFile(file)
     })
 
     return Promise.all(promises)
@@ -810,7 +805,7 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     })
     this.uppy.addPreProcessor(this.#setCompanionHeaders)
-    this.uppy.addUploader(this.upload)
+    this.uppy.addUploader(this.#upload)
   }
 
   uninstall () {
@@ -822,6 +817,6 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     })
     this.uppy.removePreProcessor(this.#setCompanionHeaders)
-    this.uppy.removeUploader(this.upload)
+    this.uppy.removeUploader(this.#upload)
   }
 }

packages/@uppy/aws-s3/src/index.js

@@ -28,6 +28,7 @@
 import BasePlugin from '@uppy/core/lib/BasePlugin.js'
 import { RateLimitedQueue, internalRateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
 import { RequestClient } from '@uppy/companion-client'
+import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters'
 
 import packageJson from '../package.json'
 import MiniXHRUpload from './MiniXHRUpload.js'
@@ -163,7 +164,7 @@ export default class AwsS3 extends BasePlugin {
       .then(assertServerError)
   }
 
-  #handleUpload = (fileIDs) => {
+  #handleUpload = async (fileIDs) => {
     /**
      * keep track of `getUploadParameters()` responses
      * so we can cancel the calls individually using just a file ID
@@ -178,10 +179,11 @@ export default class AwsS3 extends BasePlugin {
     }
     this.uppy.on('file-removed', onremove)
 
-    fileIDs.forEach((id) => {
-      const file = this.uppy.getFile(id)
-      this.uppy.emit('upload-started', file)
-    })
+    const files = this.uppy.getFilesByIds(fileIDs)
+
+    const filesFiltered = filterNonFailedFiles(files)
+    const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
+    this.uppy.emit('upload-start', filesToEmit)
 
     const getUploadParameters = this.#requests.wrapPromiseFunction((file) => {
       return this.opts.getUploadParameters(file)

packages/@uppy/core/src/Restricter.js

@@ -13,6 +13,12 @@ const defaultOptions = {
 }
 
 class RestrictionError extends Error {
+  constructor (message, { isUserFacing = true, file } = {}) {
+    super(message)
+    this.isUserFacing = isUserFacing
+    if (file != null) this.file = file // only some restriction errors are related to a particular file
+  }
+
   isRestriction = true
 }
 
@@ -30,16 +36,38 @@ class Restricter {
     }
   }
 
-  validate (file, files) {
-    const { maxFileSize, minFileSize, maxTotalFileSize, maxNumberOfFiles, allowedFileTypes } = this.getOpts().restrictions
+  // Because these operations are slow, we cannot run them for every file (if we are adding multiple files)
+  validateAggregateRestrictions (existingFiles, addingFiles) {
+    const { maxTotalFileSize, maxNumberOfFiles } = this.getOpts().restrictions
 
     if (maxNumberOfFiles) {
-      const nonGhostFiles = files.filter(f => !f.isGhost)
-      if (nonGhostFiles.length + 1 > maxNumberOfFiles) {
+      const nonGhostFiles = existingFiles.filter(f => !f.isGhost)
+      if (nonGhostFiles.length + addingFiles.length > maxNumberOfFiles) {
         throw new RestrictionError(`${this.i18n('youCanOnlyUploadX', { smart_count: maxNumberOfFiles })}`)
       }
     }
 
+    if (maxTotalFileSize) {
+      let totalFilesSize = existingFiles.reduce((total, f) => (total + f.size), 0)
+
+      for (const addingFile of addingFiles) {
+        if (addingFile.size != null) { // We can't check maxTotalFileSize if the size is unknown.
+          totalFilesSize += addingFile.size
+
+          if (totalFilesSize > maxTotalFileSize) {
+            throw new RestrictionError(this.i18n('exceedsSize', {
+              size: prettierBytes(maxTotalFileSize),
+              file: addingFile.name,
+            }))
+          }
+        }
+      }
+    }
+  }
+
+  validateSingleFile (file) {
+    const { maxFileSize, minFileSize, allowedFileTypes } = this.getOpts().restrictions
+
     if (allowedFileTypes) {
       const isCorrectFileType = allowedFileTypes.some((type) => {
         // check if this is a mime-type
@@ -57,19 +85,7 @@ class Restricter {
 
       if (!isCorrectFileType) {
         const allowedFileTypesString = allowedFileTypes.join(', ')
-        throw new RestrictionError(this.i18n('youCanOnlyUploadFileTypes', { types: allowedFileTypesString }))
-      }
-    }
-
-    // We can't check maxTotalFileSize if the size is unknown.
-    if (maxTotalFileSize && file.size != null) {
-      const totalFilesSize = files.reduce((total, f) => (total + f.size), file.size)
-
-      if (totalFilesSize > maxTotalFileSize) {
-        throw new RestrictionError(this.i18n('exceedsSize', {
-          size: prettierBytes(maxTotalFileSize),
-          file: file.name,
-        }))
+        throw new RestrictionError(this.i18n('youCanOnlyUploadFileTypes', { types: allowedFileTypesString }), { file })
       }
     }
 
@@ -78,17 +94,24 @@ class Restricter {
       throw new RestrictionError(this.i18n('exceedsSize', {
         size: prettierBytes(maxFileSize),
         file: file.name,
-      }))
+      }), { file })
     }
 
     // We can't check minFileSize if the size is unknown.
     if (minFileSize && file.size != null && file.size < minFileSize) {
       throw new RestrictionError(this.i18n('inferiorSize', {
         size: prettierBytes(minFileSize),
-      }))
+      }), { file })
     }
   }
 
+  validate (existingFiles, addingFiles) {
+    addingFiles.forEach((addingFile) => {
+      this.validateSingleFile(addingFile)
+    })
+    this.validateAggregateRestrictions(existingFiles, addingFiles)
+  }
+
   validateMinNumberOfFiles (files) {
     const { minNumberOfFiles } = this.getOpts().restrictions
     if (Object.keys(files).length < minNumberOfFiles) {