getDroppedFiles.js: handle exceptions better (#1797)

* @utils/getDroppedFiles.js - mild refactoring

* @utils/getDroppedFiles.js - added .info and .log on drop error

* @utils/getDroppedFiles.js - one more refactor

* @utils/getDroppedFiles.js - log error objects instead of strings

packages/@uppy/dashboard/src/index.js

@@ -546,7 +546,17 @@ module.exports = class Dashboard extends Plugin {
     })
 
     // 4. Add all dropped files
-    getDroppedFiles(event.dataTransfer)
+    let executedDropErrorOnce = false
+    const logDropError = (error) => {
+      this.uppy.log(error, 'error')
+
+      // In practice all drop errors are most likely the same, so let's just show one to avoid overwhelming the user
+      if (!executedDropErrorOnce) {
+        this.uppy.info(error.message, 'error')
+        executedDropErrorOnce = true
+      }
+    }
+    getDroppedFiles(event.dataTransfer, { logDropError })
       .then((files) => {
         if (files.length > 0) {
           this.uppy.log('[Dashboard] Files were dropped')

packages/@uppy/drag-drop/src/index.js

@@ -98,8 +98,11 @@ module.exports = class DragDrop extends Plugin {
     this.setPluginState({ isDraggingOver: false })
 
     // 3. Add all dropped files
-    this.uppy.log('[DragDrop] File were dropped')
-    getDroppedFiles(event.dataTransfer)
+    this.uppy.log('[DragDrop] Files were dropped')
+    const logDropError = (error) => {
+      this.uppy.log(error, 'error')
+    }
+    getDroppedFiles(event.dataTransfer, { logDropError })
       .then((files) => {
         files.forEach(this.addFile)
       })

packages/@uppy/utils/src/getDroppedFiles/index.js

@@ -1,4 +1,4 @@
-const webkitGetAsEntryApi = require('./utils/webkitGetAsEntryApi')
+const webkitGetAsEntryApi = require('./utils/webkitGetAsEntryApi/index')
 const fallbackApi = require('./utils/fallbackApi')
 
 /**
@@ -6,12 +6,14 @@ const fallbackApi = require('./utils/fallbackApi')
  * Each file has .relativePath prop appended to it (e.g. "/docs/Prague/ticket_from_prague_to_ufa.pdf") if browser supports it. Otherwise it's undefined.
  *
  * @param {DataTransfer} dataTransfer
+ * @param {Function} logDropError - a function that's called every time some folder or some file error out (e.g. because of the folder name being too long on Windows). Notice that resulting promise will always be resolved anyway.
+ *
  * @returns {Promise} - Array<File>
  */
-module.exports = function getDroppedFiles (dataTransfer) {
+module.exports = function getDroppedFiles (dataTransfer, { logDropError = () => {} } = {}) {
   // Get all files from all subdirs. Works (at least) in Chrome, Mozilla, and Safari
   if (dataTransfer.items && dataTransfer.items[0] && 'webkitGetAsEntry' in dataTransfer.items[0]) {
-    return webkitGetAsEntryApi(dataTransfer)
+    return webkitGetAsEntryApi(dataTransfer, logDropError)
   // Otherwise just return all first-order files
   } else {
     return fallbackApi(dataTransfer)

packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi.js

@@ -1,115 +0,0 @@
-const toArray = require('../../toArray')
-
-/**
- * Get the relative path from the FileEntry#fullPath, because File#webkitRelativePath is always '', at least onDrop.
- *
- * @param {FileEntry} fileEntry
- *
- * @returns {string|null} - if file is not in a folder - return null (this is to be consistent with .relativePath-s of files selected from My Device). If file is in a folder - return its fullPath, e.g. '/simpsons/hi.jpeg'.
- */
-function getRelativePath (fileEntry) {
-  // fileEntry.fullPath - "/simpsons/hi.jpeg" or undefined (for browsers that don't support it)
-  // fileEntry.name - "hi.jpeg"
-  if (!fileEntry.fullPath || fileEntry.fullPath === '/' + fileEntry.name) {
-    return null
-  } else {
-    return fileEntry.fullPath
-  }
-}
-
-/**
- * Recursive function, calls the original callback() when the directory is entirely parsed.
- *
- * @param {FileSystemDirectoryReader} directoryReader
- * @param {Array} oldEntries
- * @param {Function} callback - called with ([ all files and directories in that directoryReader ])
- */
-function readEntries (directoryReader, oldEntries, callback) {
-  directoryReader.readEntries(
-    (entries) => {
-      const newEntries = [...oldEntries, ...entries]
-      // According to the FileSystem API spec, readEntries() must be called until it calls the callback with an empty array.
-      if (entries.length) {
-        setTimeout(() => {
-          readEntries(directoryReader, newEntries, callback)
-        }, 0)
-      // Done iterating this particular directory
-      } else {
-        callback(newEntries)
-      }
-    },
-    // Make sure we resolve on error anyway
-    () =>
-      callback(oldEntries)
-  )
-}
-
-/**
- * @param {Function} resolve - function that will be called when :files array is appended with a file
- * @param {Array<File>} files - array of files to enhance
- * @param {FileSystemFileEntry} fileEntry
- */
-function addEntryToFiles (resolve, files, fileEntry) {
-  // Creates a new File object which can be used to read the file.
-  fileEntry.file(
-    (file) => {
-      file.relativePath = getRelativePath(fileEntry)
-      files.push(file)
-      resolve()
-    },
-    // Make sure we resolve on error anyway
-    () =>
-      resolve()
-  )
-}
-
-/**
- * @param {Function} resolve - function that will be called when :directoryEntry is done being recursively parsed
- * @param {Array<File>} files - array of files to enhance
- * @param {FileSystemDirectoryEntry} directoryEntry
- */
-function recursivelyAddFilesFromDirectory (resolve, files, directoryEntry) {
-  const directoryReader = directoryEntry.createReader()
-  readEntries(directoryReader, [], (entries) => {
-    const promises =
-      entries.map((entry) =>
-        createPromiseToAddFileOrParseDirectory(files, entry)
-      )
-    Promise.all(promises)
-      .then(() =>
-        resolve()
-      )
-  })
-}
-
-/**
- * @param {Array<File>} files - array of files to enhance
- * @param {(FileSystemFileEntry|FileSystemDirectoryEntry)} entry
- */
-function createPromiseToAddFileOrParseDirectory (files, entry) {
-  return new Promise((resolve) => {
-    if (entry.isFile) {
-      addEntryToFiles(resolve, files, entry)
-    } else if (entry.isDirectory) {
-      recursivelyAddFilesFromDirectory(resolve, files, entry)
-    }
-  })
-}
-
-module.exports = function webkitGetAsEntryApi (dataTransfer) {
-  const files = []
-
-  const rootPromises = []
-
-  toArray(dataTransfer.items)
-    .forEach((item) => {
-      const entry = item.webkitGetAsEntry()
-      // :entry can be null when we drop the url e.g.
-      if (entry) {
-        rootPromises.push(createPromiseToAddFileOrParseDirectory(files, entry))
-      }
-    })
-
-  return Promise.all(rootPromises)
-    .then(() => files)
-}

packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/getFilesAndDirectoriesFromDirectory.js

@@ -0,0 +1,29 @@
+/**
+ * Recursive function, calls the original callback() when the directory is entirely parsed.
+ *
+ * @param {FileSystemDirectoryReader} directoryReader
+ * @param {Array} oldEntries
+ * @param {Function} logDropError
+ * @param {Function} callback - called with ([ all files and directories in that directoryReader ])
+ */
+module.exports = function getFilesAndDirectoriesFromDirectory (directoryReader, oldEntries, logDropError, { onSuccess }) {
+  directoryReader.readEntries(
+    (entries) => {
+      const newEntries = [...oldEntries, ...entries]
+      // According to the FileSystem API spec, getFilesAndDirectoriesFromDirectory() must be called until it calls the onSuccess with an empty array.
+      if (entries.length) {
+        setTimeout(() => {
+          getFilesAndDirectoriesFromDirectory(directoryReader, newEntries, logDropError, { onSuccess })
+        }, 0)
+      // Done iterating this particular directory
+      } else {
+        onSuccess(newEntries)
+      }
+    },
+    // Make sure we resolve on error anyway, it's fine if only one directory couldn't be parsed!
+    (error) => {
+      logDropError(error)
+      onSuccess(oldEntries)
+    }
+  )
+}

packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/getRelativePath.js

@@ -0,0 +1,16 @@
+/**
+ * Get the relative path from the FileEntry#fullPath, because File#webkitRelativePath is always '', at least onDrop.
+ *
+ * @param {FileEntry} fileEntry
+ *
+ * @returns {string|null} - if file is not in a folder - return null (this is to be consistent with .relativePath-s of files selected from My Device). If file is in a folder - return its fullPath, e.g. '/simpsons/hi.jpeg'.
+ */
+module.exports = function getRelativePath (fileEntry) {
+  // fileEntry.fullPath - "/simpsons/hi.jpeg" or undefined (for browsers that don't support it)
+  // fileEntry.name - "hi.jpeg"
+  if (!fileEntry.fullPath || fileEntry.fullPath === '/' + fileEntry.name) {
+    return null
+  } else {
+    return fileEntry.fullPath
+  }
+}

packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/index.js

@@ -0,0 +1,57 @@
+const toArray = require('../../../toArray')
+const getRelativePath = require('./getRelativePath')
+const getFilesAndDirectoriesFromDirectory = require('./getFilesAndDirectoriesFromDirectory')
+
+module.exports = function webkitGetAsEntryApi (dataTransfer, logDropError) {
+  const files = []
+
+  const rootPromises = []
+
+  /**
+   * Returns a resolved promise, when :files array is enhanced
+   *
+   * @param {(FileSystemFileEntry|FileSystemDirectoryEntry)} entry
+   * @returns {Promise} - empty promise that resolves when :files is enhanced with a file
+   */
+  const createPromiseToAddFileOrParseDirectory = (entry) =>
+    new Promise((resolve) => {
+      // This is a base call
+      if (entry.isFile) {
+        // Creates a new File object which can be used to read the file.
+        entry.file(
+          (file) => {
+            file.relativePath = getRelativePath(entry)
+            files.push(file)
+            resolve()
+          },
+          // Make sure we resolve on error anyway, it's fine if only one file couldn't be read!
+          (error) => {
+            logDropError(error)
+            resolve()
+          }
+        )
+      // This is a recursive call
+      } else if (entry.isDirectory) {
+        const directoryReader = entry.createReader()
+        getFilesAndDirectoriesFromDirectory(directoryReader, [], logDropError, {
+          onSuccess: (entries) => {
+            const promises = entries.map((entry) => createPromiseToAddFileOrParseDirectory(entry))
+            Promise.all(promises).then(() => resolve())
+          }
+        })
+      }
+    })
+
+  // For each dropped item, - make sure it's a file/directory, and start deepening in!
+  toArray(dataTransfer.items)
+    .forEach((item) => {
+      const entry = item.webkitGetAsEntry()
+      // :entry can be null when we drop the url e.g.
+      if (entry) {
+        rootPromises.push(createPromiseToAddFileOrParseDirectory(entry))
+      }
+    })
+
+  return Promise.all(rootPromises)
+    .then(() => files)
+}

packages/@uppy/utils/types/index.d.ts

@@ -131,7 +131,7 @@ declare module '@uppy/utils/lib/toArray' {
 }
 
 declare module '@uppy/utils/lib/getDroppedFiles' {
-  export default function getDroppedFiles(dataTransfer: DataTransfer): Promise<File[]>;
+  export default function getDroppedFiles(dataTransfer: DataTransfer, options?: object): Promise<File[]>;
 }
 
 declare module '@uppy/utils' {