From 9b093ae28832c870f873202a8fcf0f7256cbaca7 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 28 Mar 2023 23:15:46 +0900 Subject: [PATCH 01/24] fix race condtion when adding files don't call addFolder from listAllFiles because that would call addFile before all folders were loaded also remove unused selectedFolders state --- packages/@uppy/provider-views/package.json | 1 + .../src/ProviderView/ProviderView.jsx | 69 ++++++++----------- yarn.lock | 43 ++++++++++++ 3 files changed, 71 insertions(+), 42 deletions(-) diff --git a/packages/@uppy/provider-views/package.json b/packages/@uppy/provider-views/package.json index ec4f4f3b0f..cca2921d6f 100644 --- a/packages/@uppy/provider-views/package.json +++ b/packages/@uppy/provider-views/package.json @@ -22,6 +22,7 @@ "dependencies": { "@uppy/utils": "workspace:^", "classnames": "^2.2.6", + "p-map": "^5.5.0", "preact": "^10.5.13" }, "peerDependencies": { diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index e7af61f246..2e4154d6ba 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,4 +1,5 @@ import { h } from 'preact' +import pMap from 'p-map' import AuthView from './AuthView.jsx' import Header from './Header.jsx' @@ -58,7 +59,6 @@ export default class ProviderView extends View { this.logout = this.logout.bind(this) this.handleAuth = this.handleAuth.bind(this) this.handleScroll = this.handleScroll.bind(this) - this.listAllFiles = this.listAllFiles.bind(this) this.donePicking = this.donePicking.bind(this) // Visual @@ -173,20 +173,9 @@ export default class ProviderView extends View { * Uses separated state while folder contents are being fetched and * mantains list of selected folders, which are separated from files. */ - addFolder (folder) { - const folderId = this.providerFileToId(folder) - const folders = { ...this.plugin.getPluginState().selectedFolders } - - if (folderId in folders && folders[folderId].loading) { - return - } - - folders[folderId] = { loading: true, files: [] } - - this.plugin.setPluginState({ selectedFolders: { ...folders } }) - - // eslint-disable-next-line consistent-return - return this.listAllFiles(folder.requestPath).then((files) => { + async addFolder (folder) { + try { + const files = await this.recursivelyListAllFiles(folder.requestPath) let count = 0 // If the same folder is added again, we don't want to send @@ -204,13 +193,7 @@ export default class ProviderView extends View { files.forEach((file) => this.addFile(file)) } - const ids = files.map(this.providerFileToId) - - folders[folderId] = { - loading: false, - files: ids, - } - this.plugin.setPluginState({ selectedFolders: folders, filterInput: '' }) + this.plugin.setPluginState({ filterInput: '' }) let message @@ -227,12 +210,9 @@ export default class ProviderView extends View { } this.plugin.uppy.info(message) - }).catch((e) => { - const selectedFolders = { ...this.plugin.getPluginState().selectedFolders } - delete selectedFolders[folderId] - this.plugin.setPluginState({ selectedFolders }) - this.handleError(e) - }) + } catch (err) { + this.handleError(err) + } } async handleAuth () { @@ -296,20 +276,26 @@ export default class ProviderView extends View { } } - async listAllFiles (path, files = null) { - files = files || [] // eslint-disable-line no-param-reassign - const res = await this.provider.list(path) - res.items.forEach((item) => { - if (!item.isFolder) { - files.push(item) - } else { - this.addFolder(item) - } - }) - const moreFiles = res.nextPagePath - if (moreFiles) { - return this.listAllFiles(moreFiles, files) + async recursivelyListAllFiles (path, files = []) { + for (;;) { + let curPath = path + const res = await this.provider.list(curPath) + + // limit concurrency (because what if we have a folder with 1000 folders inside!) + // todo this might still lead to a memory run-off + await pMap(res.items, async (item) => { + if (item.isFolder) { + await this.recursivelyListAllFiles(item.requestPath, files) + } else { + files.push(item) + } + }, { concurrency: 10 }) + + // need to repeat this call until there are no more pages + if (!res.nextPagePath) break + curPath = res.nextPagePath } + return files } @@ -363,7 +349,6 @@ export default class ProviderView extends View { filterQuery: this.filterQuery, logout: this.logout, handleScroll: this.handleScroll, - listAllFiles: this.listAllFiles, done: this.donePicking, cancel: this.cancelPicking, headerComponent: Header(headerProps), diff --git a/yarn.lock b/yarn.lock index f2c6a8fe3b..346075496c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8954,6 +8954,7 @@ __metadata: dependencies: "@uppy/utils": "workspace:^" classnames: ^2.2.6 + p-map: ^5.5.0 preact: ^10.5.13 peerDependencies: "@uppy/core": "workspace:^" @@ -10067,6 +10068,16 @@ __metadata: languageName: node linkType: hard +"aggregate-error@npm:^4.0.0": + version: 4.0.1 + resolution: "aggregate-error@npm:4.0.1" + dependencies: + clean-stack: ^4.0.0 + indent-string: ^5.0.0 + checksum: bb3ffdfd13447800fff237c2cba752c59868ee669104bb995dfbbe0b8320e967d679e683dabb640feb32e4882d60258165cde0baafc4cd467cc7d275a13ad6b5 + languageName: node + linkType: hard + "airbnb-js-shims@npm:^2.2.1": version: 2.2.1 resolution: "airbnb-js-shims@npm:2.2.1" @@ -12553,6 +12564,15 @@ __metadata: languageName: node linkType: hard +"clean-stack@npm:^4.0.0": + version: 4.2.0 + resolution: "clean-stack@npm:4.2.0" + dependencies: + escape-string-regexp: 5.0.0 + checksum: 373f656a31face5c615c0839213b9b542a0a48057abfb1df66900eab4dc2a5c6097628e4a0b5aa559cdfc4e66f8a14ea47be9681773165a44470ef5fb8ccc172 + languageName: node + linkType: hard + "cli-boxes@npm:^2.2.1": version: 2.2.1 resolution: "cli-boxes@npm:2.2.1" @@ -15846,6 +15866,13 @@ __metadata: languageName: node linkType: hard +"escape-string-regexp@npm:5.0.0": + version: 5.0.0 + resolution: "escape-string-regexp@npm:5.0.0" + checksum: 20daabe197f3cb198ec28546deebcf24b3dbb1a5a269184381b3116d12f0532e06007f4bc8da25669d6a7f8efb68db0758df4cd981f57bc5b57f521a3e12c59e + languageName: node + linkType: hard + "escape-string-regexp@npm:^1.0.5": version: 1.0.5 resolution: "escape-string-regexp@npm:1.0.5" @@ -19463,6 +19490,13 @@ __metadata: languageName: node linkType: hard +"indent-string@npm:^5.0.0": + version: 5.0.0 + resolution: "indent-string@npm:5.0.0" + checksum: e466c27b6373440e6d84fbc19e750219ce25865cb82d578e41a6053d727e5520dc5725217d6eb1cc76005a1bb1696a0f106d84ce7ebda3033b963a38583fb3b3 + languageName: node + linkType: hard + "indexes-of@npm:^1.0.1": version: 1.0.1 resolution: "indexes-of@npm:1.0.1" @@ -25805,6 +25839,15 @@ __metadata: languageName: node linkType: hard +"p-map@npm:^5.5.0": + version: 5.5.0 + resolution: "p-map@npm:5.5.0" + dependencies: + aggregate-error: ^4.0.0 + checksum: 065cb6fca6b78afbd070dd9224ff160dc23eea96e57863c09a0c8ea7ce921043f76854be7ee0abc295cff1ac9adcf700e79a1fbe3b80b625081087be58e7effb + languageName: node + linkType: hard + "p-retry@npm:^4.5.0": version: 4.6.2 resolution: "p-retry@npm:4.6.2" From 25f6228c47ef4f05ab374f3ac781db9e3d4ca988 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 28 Mar 2023 23:56:44 +0900 Subject: [PATCH 02/24] fix also the case of adding multiple folders --- .../src/ProviderView/ProviderView.jsx | 111 +++++++++--------- 1 file changed, 57 insertions(+), 54 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 2e4154d6ba..e4fcaf5513 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -167,54 +167,6 @@ export default class ProviderView extends View { this.plugin.setPluginState({ ...state, filterInput: e ? e.target.value : '' }) } - /** - * Adds all files found inside of specified folder. - * - * Uses separated state while folder contents are being fetched and - * mantains list of selected folders, which are separated from files. - */ - async addFolder (folder) { - try { - const files = await this.recursivelyListAllFiles(folder.requestPath) - let count = 0 - - // If the same folder is added again, we don't want to send - // X amount of duplicate file notifications, we want to say - // the folder was already added. This checks if all files are duplicate, - // if that's the case, we don't add the files. - files.forEach(file => { - const id = this.providerFileToId(file) - if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { - count++ - } - }) - - if (count > 0) { - files.forEach((file) => this.addFile(file)) - } - - this.plugin.setPluginState({ filterInput: '' }) - - let message - - if (count === 0) { - message = this.plugin.uppy.i18n('folderAlreadyAdded', { - folder: folder.name, - }) - } else if (files.length) { - message = this.plugin.uppy.i18n('folderAdded', { - smart_count: count, folder: folder.name, - }) - } else { - message = this.plugin.uppy.i18n('emptyFolderAdded') - } - - this.plugin.uppy.info(message) - } catch (err) { - this.handleError(err) - } - } - async handleAuth () { await this.provider.ensurePreAuth() @@ -301,14 +253,65 @@ export default class ProviderView extends View { donePicking () { const { currentSelection } = this.plugin.getPluginState() - const promises = currentSelection.map((file) => { - if (file.isFolder) { - return this.addFolder(file) + + const addSelection = async () => { + try { + const allFiles = [] + const foldersAdded = [] + + // eslint-disable-next-line no-unreachable-loop + for (const file of currentSelection) { + if (file.isFolder) { + const folder = file + const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) + allFiles.push(...filesInFolder) + + let numNewFiles = 0 + + // If the same folder is added again, we don't want to send + // X amount of duplicate file notifications, we want to say + // the folder was already added. This checks if all files are duplicate, + // if that's the case, we don't add the files. + filesInFolder.forEach((fileInFolder) => { + const id = this.providerFileToId(fileInFolder) + if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { + numNewFiles++ + } + }) + + foldersAdded.push({ numNewFiles, name: folder.name }) + } else { + await this.addFile(file) + } + } + + allFiles.forEach((file) => this.addFile(file)) + + this.plugin.setPluginState({ filterInput: '' }) + + let message + + for (const { name, numNewFiles } of foldersAdded) { + if (numNewFiles === 0) { + message = this.plugin.uppy.i18n('folderAlreadyAdded', { + folder: name, + }) + } else if (numNewFiles) { + message = this.plugin.uppy.i18n('folderAdded', { + smart_count: numNewFiles, folder: name, + }) + } else { + message = this.plugin.uppy.i18n('emptyFolderAdded') + } + + this.plugin.uppy.info(message) + } + } catch (err) { + this.handleError(err) } - return this.addFile(file) - }) + } - this.sharedHandler.loaderWrapper(Promise.all(promises), () => { + this.sharedHandler.loaderWrapper(addSelection(), () => { this.clearSelection() }, () => {}) } From cd121f3b26e6b919c79e5cea6e0339e9f374e9a4 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 11:26:01 +0900 Subject: [PATCH 03/24] fix todo: remove SharedHandler --- .../src/ProviderView/ProviderView.jsx | 7 +- .../SearchProviderView/SearchProviderView.jsx | 6 +- .../@uppy/provider-views/src/SharedHandler.js | 100 ------------------ packages/@uppy/provider-views/src/View.js | 94 +++++++++++++++- 4 files changed, 95 insertions(+), 112 deletions(-) delete mode 100644 packages/@uppy/provider-views/src/SharedHandler.js diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index e4fcaf5513..9b14194c46 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -101,7 +101,7 @@ export default class ProviderView extends View { * @returns {Promise} Folders/files in folder */ getFolder (id, name) { - return this.sharedHandler.loaderWrapper( + return this.loaderWrapper( this.provider.list(id), (res) => { const folders = [] @@ -311,7 +311,7 @@ export default class ProviderView extends View { } } - this.sharedHandler.loaderWrapper(addSelection(), () => { + this.loaderWrapper(addSelection(), () => { this.clearSelection() }, () => {}) } @@ -325,7 +325,7 @@ export default class ProviderView extends View { const targetViewOptions = { ...this.opts, ...viewOptions } const { files, folders, filterInput, loading, currentSelection } = this.plugin.getPluginState() - const { isChecked, toggleCheckbox, recordShiftKeyPress, filterItems } = this.sharedHandler + const { isChecked, toggleCheckbox, recordShiftKeyPress, filterItems } = this const hasInput = filterInput !== '' const headerProps = { showBreadcrumbs: targetViewOptions.showBreadcrumbs, @@ -348,7 +348,6 @@ export default class ProviderView extends View { username: this.username, getNextFolder: this.getNextFolder, getFolder: this.getFolder, - filterItems: this.sharedHandler.filterItems, filterQuery: this.filterQuery, logout: this.logout, handleScroll: this.handleScroll, diff --git a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx index cf8b1d8eb9..2b6686108d 100644 --- a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx +++ b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx @@ -86,7 +86,7 @@ export default class SearchProviderView extends View { return undefined } - return this.sharedHandler.loaderWrapper( + return this.loaderWrapper( this.provider.search(query), (res) => { this.#updateFilesAndInputMode(res, []) @@ -122,7 +122,7 @@ export default class SearchProviderView extends View { const { currentSelection } = this.plugin.getPluginState() const promises = currentSelection.map((file) => this.addFile(file)) - this.sharedHandler.loaderWrapper(Promise.all(promises), () => { + this.loaderWrapper(Promise.all(promises), () => { this.clearSelection() }, () => {}) } @@ -136,7 +136,7 @@ export default class SearchProviderView extends View { const targetViewOptions = { ...this.opts, ...viewOptions } const { files, folders, filterInput, loading, currentSelection } = this.plugin.getPluginState() - const { isChecked, toggleCheckbox, filterItems } = this.sharedHandler + const { isChecked, toggleCheckbox, filterItems } = this const hasInput = filterInput !== '' const browserProps = { diff --git a/packages/@uppy/provider-views/src/SharedHandler.js b/packages/@uppy/provider-views/src/SharedHandler.js deleted file mode 100644 index e7af174116..0000000000 --- a/packages/@uppy/provider-views/src/SharedHandler.js +++ /dev/null @@ -1,100 +0,0 @@ -import remoteFileObjToLocal from '@uppy/utils/lib/remoteFileObjToLocal' - -export default class SharedHandler { - constructor (plugin) { - this.plugin = plugin - this.filterItems = this.filterItems.bind(this) - this.toggleCheckbox = this.toggleCheckbox.bind(this) - this.recordShiftKeyPress = this.recordShiftKeyPress.bind(this) - this.isChecked = this.isChecked.bind(this) - this.loaderWrapper = this.loaderWrapper.bind(this) - } - - filterItems (items) { - const state = this.plugin.getPluginState() - if (!state.filterInput || state.filterInput === '') { - return items - } - return items.filter((folder) => { - return folder.name.toLowerCase().indexOf(state.filterInput.toLowerCase()) !== -1 - }) - } - - recordShiftKeyPress (e) { - this.isShiftKeyPressed = e.shiftKey - } - - /** - * Toggles file/folder checkbox to on/off state while updating files list. - * - * Note that some extra complexity comes from supporting shift+click to - * toggle multiple checkboxes at once, which is done by getting all files - * in between last checked file and current one. - */ - toggleCheckbox (e, file) { - e.stopPropagation() - e.preventDefault() - e.currentTarget.focus() - const { folders, files } = this.plugin.getPluginState() - const items = this.filterItems(folders.concat(files)) - // Shift-clicking selects a single consecutive list of items - // starting at the previous click and deselects everything else. - if (this.lastCheckbox && this.isShiftKeyPressed) { - const prevIndex = items.indexOf(this.lastCheckbox) - const currentIndex = items.indexOf(file) - const currentSelection = (prevIndex < currentIndex) - ? items.slice(prevIndex, currentIndex + 1) - : items.slice(currentIndex, prevIndex + 1) - const reducedCurrentSelection = [] - - // Check restrictions on each file in currentSelection, - // reduce it to only contain files that pass restrictions - for (const item of currentSelection) { - const { uppy } = this.plugin - const restrictionError = uppy.validateRestrictions( - remoteFileObjToLocal(item), - [...uppy.getFiles(), ...reducedCurrentSelection], - ) - - if (!restrictionError) { - reducedCurrentSelection.push(item) - } else { - uppy.info({ message: restrictionError.message }, 'error', uppy.opts.infoTimeout) - } - } - this.plugin.setPluginState({ currentSelection: reducedCurrentSelection }) - return - } - - this.lastCheckbox = file - const { currentSelection } = this.plugin.getPluginState() - if (this.isChecked(file)) { - this.plugin.setPluginState({ - currentSelection: currentSelection.filter((item) => item.id !== file.id), - }) - } else { - this.plugin.setPluginState({ - currentSelection: currentSelection.concat([file]), - }) - } - } - - isChecked (file) { - const { currentSelection } = this.plugin.getPluginState() - // comparing id instead of the file object, because the reference to the object - // changes when we switch folders, and the file list is updated - return currentSelection.some((item) => item.id === file.id) - } - - loaderWrapper (promise, then, catch_) { - promise - .then((result) => { - this.plugin.setPluginState({ loading: false }) - then(result) - }).catch((err) => { - this.plugin.setPluginState({ loading: false }) - catch_(err) - }) - this.plugin.setPluginState({ loading: true }) - } -} diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index 1ed2eae584..99702ae1cd 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -1,16 +1,12 @@ import getFileType from '@uppy/utils/lib/getFileType' import isPreviewSupported from '@uppy/utils/lib/isPreviewSupported' import generateFileID from '@uppy/utils/lib/generateFileID' - -// TODO: now that we have a shared `View` class, -// `SharedHandler` could be cleaned up and moved into here -import SharedHandler from './SharedHandler.js' +import remoteFileObjToLocal from '@uppy/utils/lib/remoteFileObjToLocal' export default class View { constructor (plugin, opts) { this.plugin = plugin this.provider = opts.provider - this.sharedHandler = new SharedHandler(plugin) this.isHandlingScroll = false @@ -117,4 +113,92 @@ export default class View { return false } } + + filterItems = (items) => { + const state = this.plugin.getPluginState() + if (!state.filterInput || state.filterInput === '') { + return items + } + return items.filter((folder) => { + return folder.name.toLowerCase().indexOf(state.filterInput.toLowerCase()) !== -1 + }) + } + + recordShiftKeyPress = (e) => { + this.isShiftKeyPressed = e.shiftKey + } + + /** + * Toggles file/folder checkbox to on/off state while updating files list. + * + * Note that some extra complexity comes from supporting shift+click to + * toggle multiple checkboxes at once, which is done by getting all files + * in between last checked file and current one. + */ + toggleCheckbox = (e, file) => { + e.stopPropagation() + e.preventDefault() + e.currentTarget.focus() + const { folders, files } = this.plugin.getPluginState() + const items = this.filterItems(folders.concat(files)) + // Shift-clicking selects a single consecutive list of items + // starting at the previous click and deselects everything else. + if (this.lastCheckbox && this.isShiftKeyPressed) { + const prevIndex = items.indexOf(this.lastCheckbox) + const currentIndex = items.indexOf(file) + const currentSelection = (prevIndex < currentIndex) + ? items.slice(prevIndex, currentIndex + 1) + : items.slice(currentIndex, prevIndex + 1) + const reducedCurrentSelection = [] + + // Check restrictions on each file in currentSelection, + // reduce it to only contain files that pass restrictions + for (const item of currentSelection) { + const { uppy } = this.plugin + const restrictionError = uppy.validateRestrictions( + remoteFileObjToLocal(item), + [...uppy.getFiles(), ...reducedCurrentSelection], + ) + + if (!restrictionError) { + reducedCurrentSelection.push(item) + } else { + uppy.info({ message: restrictionError.message }, 'error', uppy.opts.infoTimeout) + } + } + this.plugin.setPluginState({ currentSelection: reducedCurrentSelection }) + return + } + + this.lastCheckbox = file + const { currentSelection } = this.plugin.getPluginState() + if (this.isChecked(file)) { + this.plugin.setPluginState({ + currentSelection: currentSelection.filter((item) => item.id !== file.id), + }) + } else { + this.plugin.setPluginState({ + currentSelection: currentSelection.concat([file]), + }) + } + } + + isChecked = (file) => { + const { currentSelection } = this.plugin.getPluginState() + // comparing id instead of the file object, because the reference to the object + // changes when we switch folders, and the file list is updated + return currentSelection.some((item) => item.id === file.id) + } + + loaderWrapper = (promise, then, catch_) => { + promise + .then((result) => { + this.plugin.setPluginState({ loading: false }) + then(result) + }).catch((err) => { + this.plugin.setPluginState({ loading: false }) + catch_(err) + }) + this.plugin.setPluginState({ loading: true }) + } } From ea3cb23350494f11e3ef80267ee966297f0f064d Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 12:17:52 +0900 Subject: [PATCH 04/24] remove loaderWrapper --- .../src/ProviderView/ProviderView.jsx | 155 +++++++++--------- .../SearchProviderView/SearchProviderView.jsx | 27 ++- packages/@uppy/provider-views/src/View.js | 12 +- 3 files changed, 93 insertions(+), 101 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 9b14194c46..b2ba0d6dea 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -100,29 +100,31 @@ export default class ProviderView extends View { * @param {string} id Folder id * @returns {Promise} Folders/files in folder */ - getFolder (id, name) { - return this.loaderWrapper( - this.provider.list(id), - (res) => { - const folders = [] - const files = [] - let updatedDirectories - - const state = this.plugin.getPluginState() - const index = state.directories.findIndex((dir) => id === dir.id) - - if (index !== -1) { - updatedDirectories = state.directories.slice(0, index + 1) - } else { - updatedDirectories = state.directories.concat([{ id, title: name }]) - } + async getFolder (id, name) { + this.setLoading(true) + try { + const res = await this.provider.list(id) + const folders = [] + const files = [] + let updatedDirectories + + const state = this.plugin.getPluginState() + const index = state.directories.findIndex((dir) => id === dir.id) + + if (index !== -1) { + updatedDirectories = state.directories.slice(0, index + 1) + } else { + updatedDirectories = state.directories.concat([{ id, title: name }]) + } - this.username = res.username || this.username - this.#updateFilesAndFolders(res, files, folders) - this.plugin.setPluginState({ directories: updatedDirectories, filterInput: '' }) - }, - this.handleError, - ) + this.username = res.username || this.username + this.#updateFilesAndFolders(res, files, folders) + this.plugin.setPluginState({ directories: updatedDirectories, filterInput: '' }) + } catch (err) { + this.handleError(err) + } finally { + this.setLoading(false) + } } /** @@ -251,69 +253,68 @@ export default class ProviderView extends View { return files } - donePicking () { - const { currentSelection } = this.plugin.getPluginState() - - const addSelection = async () => { - try { - const allFiles = [] - const foldersAdded = [] - - // eslint-disable-next-line no-unreachable-loop - for (const file of currentSelection) { - if (file.isFolder) { - const folder = file - const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) - allFiles.push(...filesInFolder) - - let numNewFiles = 0 - - // If the same folder is added again, we don't want to send - // X amount of duplicate file notifications, we want to say - // the folder was already added. This checks if all files are duplicate, - // if that's the case, we don't add the files. - filesInFolder.forEach((fileInFolder) => { - const id = this.providerFileToId(fileInFolder) - if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { - numNewFiles++ - } - }) - - foldersAdded.push({ numNewFiles, name: folder.name }) - } else { - await this.addFile(file) - } + async donePicking () { + this.setLoading(true) + try { + const { currentSelection } = this.plugin.getPluginState() + + const allFiles = [] + const foldersAdded = [] + + // eslint-disable-next-line no-unreachable-loop + for (const file of currentSelection) { + if (file.isFolder) { + const folder = file + const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) + allFiles.push(...filesInFolder) + + let numNewFiles = 0 + + // If the same folder is added again, we don't want to send + // X amount of duplicate file notifications, we want to say + // the folder was already added. This checks if all files are duplicate, + // if that's the case, we don't add the files. + filesInFolder.forEach((fileInFolder) => { + const id = this.providerFileToId(fileInFolder) + if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { + numNewFiles++ + } + }) + + foldersAdded.push({ numNewFiles, name: folder.name }) + } else { + await this.addFile(file) } + } - allFiles.forEach((file) => this.addFile(file)) - - this.plugin.setPluginState({ filterInput: '' }) + allFiles.forEach((file) => this.addFile(file)) - let message + this.plugin.setPluginState({ filterInput: '' }) - for (const { name, numNewFiles } of foldersAdded) { - if (numNewFiles === 0) { - message = this.plugin.uppy.i18n('folderAlreadyAdded', { - folder: name, - }) - } else if (numNewFiles) { - message = this.plugin.uppy.i18n('folderAdded', { - smart_count: numNewFiles, folder: name, - }) - } else { - message = this.plugin.uppy.i18n('emptyFolderAdded') - } + let message - this.plugin.uppy.info(message) + for (const { name, numNewFiles } of foldersAdded) { + if (numNewFiles === 0) { + message = this.plugin.uppy.i18n('folderAlreadyAdded', { + folder: name, + }) + } else if (numNewFiles) { + message = this.plugin.uppy.i18n('folderAdded', { + smart_count: numNewFiles, folder: name, + }) + } else { + message = this.plugin.uppy.i18n('emptyFolderAdded') } - } catch (err) { - this.handleError(err) + + this.plugin.uppy.info(message) + + this.clearSelection() } + } catch (err) { + this.handleError(err) + } finally { + this.setLoading(false) } - - this.loaderWrapper(addSelection(), () => { - this.clearSelection() - }, () => {}) } render (state, viewOptions = {}) { diff --git a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx index 2b6686108d..10af4beaa2 100644 --- a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx +++ b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx @@ -79,20 +79,22 @@ export default class SearchProviderView extends View { }) } - search (query) { + async search (query) { const { searchTerm } = this.plugin.getPluginState() if (query && query === searchTerm) { // no need to search again as this is the same as the previous search - return undefined + return } - return this.loaderWrapper( - this.provider.search(query), - (res) => { - this.#updateFilesAndInputMode(res, []) - }, - this.handleError, - ) + this.setLoading(true) + try { + const res = await this.provider.search(query) + this.#updateFilesAndInputMode(res, []) + } catch (err) { + this.handleError(err) + } finally { + this.setLoading(false) + } } triggerSearchInput () { @@ -120,11 +122,8 @@ export default class SearchProviderView extends View { donePicking () { const { currentSelection } = this.plugin.getPluginState() - const promises = currentSelection.map((file) => this.addFile(file)) - - this.loaderWrapper(Promise.all(promises), () => { - this.clearSelection() - }, () => {}) + currentSelection.forEach((file) => this.addFile(file)) + this.clearSelection() } render (state, viewOptions = {}) { diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index 99702ae1cd..61861fde0b 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -190,15 +190,7 @@ export default class View { return currentSelection.some((item) => item.id === file.id) } - loaderWrapper = (promise, then, catch_) => { - promise - .then((result) => { - this.plugin.setPluginState({ loading: false }) - then(result) - }).catch((err) => { - this.plugin.setPluginState({ loading: false }) - catch_(err) - }) - this.plugin.setPluginState({ loading: true }) + setLoading (loading) { + this.plugin.setPluginState({ loading }) } } From c80f11ad20fb1edc897be4baba0e4f6eb5ba0022 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 12:26:45 +0900 Subject: [PATCH 05/24] fix logic --- .../src/ProviderView/ProviderView.jsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index b2ba0d6dea..dd85e7b6f9 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -281,9 +281,9 @@ export default class ProviderView extends View { } }) - foldersAdded.push({ numNewFiles, name: folder.name }) + foldersAdded.push({ numFiles: filesInFolder.length, numNewFiles, name: folder.name }) } else { - await this.addFile(file) + this.addFile(file) } } @@ -293,17 +293,17 @@ export default class ProviderView extends View { let message - for (const { name, numNewFiles } of foldersAdded) { - if (numNewFiles === 0) { + for (const { name, numFiles, numNewFiles } of foldersAdded) { + if (numFiles === 0) { + message = this.plugin.uppy.i18n('emptyFolderAdded') + } else if (numNewFiles === 0) { message = this.plugin.uppy.i18n('folderAlreadyAdded', { folder: name, }) - } else if (numNewFiles) { + } else { message = this.plugin.uppy.i18n('folderAdded', { smart_count: numNewFiles, folder: name, }) - } else { - message = this.plugin.uppy.i18n('emptyFolderAdded') } this.plugin.uppy.info(message) From c533c90af22c4a55d9049010bff6193de9021745 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 12:31:23 +0900 Subject: [PATCH 06/24] fix the last race condition --- packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index dd85e7b6f9..c251896f01 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -283,7 +283,7 @@ export default class ProviderView extends View { foldersAdded.push({ numFiles: filesInFolder.length, numNewFiles, name: folder.name }) } else { - this.addFile(file) + allFiles.push(file) } } From cd5d09945f871af188321cde81abc640adcd219a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 13:18:53 +0900 Subject: [PATCH 07/24] fix broken duplicate file check --- packages/@uppy/core/src/Uppy.js | 19 +++++++++---- .../src/ProviderView/ProviderView.jsx | 19 ++++++------- packages/@uppy/provider-views/src/View.js | 28 +++++++++++-------- 3 files changed, 39 insertions(+), 27 deletions(-) diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index 852eee0597..af6e7aec8c 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -458,12 +458,9 @@ class Uppy { const fileName = getFileName(fileType, fileDescriptor) const fileExtension = getFileNameAndExtension(fileName).extension const isRemote = Boolean(fileDescriptor.isRemote) - const fileID = generateFileID({ - ...fileDescriptor, - type: fileType, - }) + const id = generateFileId2(fileDescriptor) - if (this.checkIfFileAlreadyExists(fileID)) { + if (this.checkIfFileAlreadyExists(id)) { const error = new RestrictionError(this.i18n('noDuplicates', { fileName })) this.#informAndEmit(error, fileDescriptor) throw error @@ -478,7 +475,7 @@ class Uppy { let newFile = { source: fileDescriptor.source || '', - id: fileID, + id, name: fileName, extension: fileExtension || '', meta: { @@ -1573,4 +1570,14 @@ class Uppy { } } +// todo come up with a better name or remove the need for two different generators +export function generateFileId2 (file) { + const fileType = getFileType(file) + + return generateFileID({ + ...file, + type: fileType, + }) +} + export default Uppy diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index c251896f01..e4e9c2cdd0 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,6 +1,8 @@ import { h } from 'preact' import pMap from 'p-map' +import { generateFileId2 } from '@uppy/core/src/Uppy.js' + import AuthView from './AuthView.jsx' import Header from './Header.jsx' import Browser from '../Browser.jsx' @@ -266,22 +268,20 @@ export default class ProviderView extends View { if (file.isFolder) { const folder = file const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) - allFiles.push(...filesInFolder) - - let numNewFiles = 0 // If the same folder is added again, we don't want to send // X amount of duplicate file notifications, we want to say // the folder was already added. This checks if all files are duplicate, // if that's the case, we don't add the files. - filesInFolder.forEach((fileInFolder) => { - const id = this.providerFileToId(fileInFolder) - if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { - numNewFiles++ - } + const newFilesInFolder = filesInFolder.filter((fileInFolder) => { + const tagFile = this.getTagFile(fileInFolder) + const id = generateFileId2(tagFile) + return !this.plugin.uppy.checkIfFileAlreadyExists(id) }) - foldersAdded.push({ numFiles: filesInFolder.length, numNewFiles, name: folder.name }) + allFiles.push(...newFilesInFolder) + + foldersAdded.push({ numFiles: filesInFolder.length, numNewFiles: newFilesInFolder.length, name: folder.name }) } else { allFiles.push(file) } @@ -307,7 +307,6 @@ export default class ProviderView extends View { } this.plugin.uppy.info(message) - this.clearSelection() } } catch (err) { diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index 61861fde0b..ff31e6ad3f 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -3,6 +3,14 @@ import isPreviewSupported from '@uppy/utils/lib/isPreviewSupported' import generateFileID from '@uppy/utils/lib/generateFileID' import remoteFileObjToLocal from '@uppy/utils/lib/remoteFileObjToLocal' +function providerFileToId (file) { + return generateFileID({ + data: file, + name: file.name || file.id, + type: file.mimetype, + }) +} + export default class View { constructor (plugin, opts) { this.plugin = plugin @@ -17,15 +25,6 @@ export default class View { this.cancelPicking = this.cancelPicking.bind(this) } - // eslint-disable-next-line class-methods-use-this - providerFileToId (file) { - return generateFileID({ - data: file, - name: file.name || file.id, - type: file.mimetype, - }) - } - preFirstRender () { this.plugin.setPluginState({ didFirstRender: true }) this.plugin.onFirstRender() @@ -66,9 +65,10 @@ export default class View { uppy.info({ message, details: error.toString() }, 'error', 5000) } - addFile (file) { + // todo document what is a "tagFile" or get rid of this concept + getTagFile (file) { const tagFile = { - id: this.providerFileToId(file), + id: providerFileToId(file), // todo what's this used for? can be removed? source: this.plugin.id, data: file, name: file.name || file.id, @@ -101,6 +101,12 @@ export default class View { if (file.author.url) tagFile.meta.authorUrl = file.author.url } + return tagFile + } + + addFile (file) { + const tagFile = this.getTagFile(file) + this.plugin.uppy.log('Adding remote file') try { From 67023a87d810fa00eb172373f546aa867d001c29 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 13:21:37 +0900 Subject: [PATCH 08/24] fix logic --- .../@uppy/provider-views/src/ProviderView/ProviderView.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index e4e9c2cdd0..29e80989d5 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -291,9 +291,8 @@ export default class ProviderView extends View { this.plugin.setPluginState({ filterInput: '' }) - let message - for (const { name, numFiles, numNewFiles } of foldersAdded) { + let message if (numFiles === 0) { message = this.plugin.uppy.i18n('emptyFolderAdded') } else if (numNewFiles === 0) { @@ -307,8 +306,9 @@ export default class ProviderView extends View { } this.plugin.uppy.info(message) - this.clearSelection() } + + this.clearSelection() } catch (err) { this.handleError(err) } finally { From 80bd2937e2b2ade5ae0273f2f94d97c59eb0c0bc Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 13:26:22 +0900 Subject: [PATCH 09/24] prettiyfy loop --- .../provider-views/src/ProviderView/ProviderView.jsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 29e80989d5..0e2353700f 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -233,22 +233,23 @@ export default class ProviderView extends View { } async recursivelyListAllFiles (path, files = []) { - for (;;) { - let curPath = path + let curPath = path + + // need to repeat the list call until there are no more pages + while (curPath) { const res = await this.provider.list(curPath) // limit concurrency (because what if we have a folder with 1000 folders inside!) // todo this might still lead to a memory run-off await pMap(res.items, async (item) => { if (item.isFolder) { + // recursively call self for folder await this.recursivelyListAllFiles(item.requestPath, files) } else { files.push(item) } }, { concurrency: 10 }) - // need to repeat this call until there are no more pages - if (!res.nextPagePath) break curPath = res.nextPagePath } From 4060019c359c529415118ca073c31b373e737f90 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 13:52:02 +0900 Subject: [PATCH 10/24] add user feedback so they know that something is happening --- packages/@uppy/core/src/locale.js | 1 + packages/@uppy/provider-views/src/Loader.jsx | 3 ++- .../src/ProviderView/ProviderView.jsx | 15 ++++++++++----- .../src/SearchProviderView/SearchProviderView.jsx | 2 +- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/@uppy/core/src/locale.js b/packages/@uppy/core/src/locale.js index 90bcaa9f48..7c62904bb8 100644 --- a/packages/@uppy/core/src/locale.js +++ b/packages/@uppy/core/src/locale.js @@ -49,6 +49,7 @@ export default { enterTextToSearch: 'Enter text to search for images', search: 'Search', emptyFolderAdded: 'No files were added from empty folder', + traversingSubfolders: 'Traversing %{numSubFolders} subfolder(s) inside "%{folder}"', folderAlreadyAdded: 'The folder "%{folder}" was already added', folderAdded: { 0: 'Added %{smart_count} file from %{folder}', diff --git a/packages/@uppy/provider-views/src/Loader.jsx b/packages/@uppy/provider-views/src/Loader.jsx index 0572ad2aae..57a6443663 100644 --- a/packages/@uppy/provider-views/src/Loader.jsx +++ b/packages/@uppy/provider-views/src/Loader.jsx @@ -1,9 +1,10 @@ import { h } from 'preact' -export default ({ i18n }) => { +export default ({ i18n, loading }) => { return (
{i18n('loading')} + {typeof loading === 'string' && {loading}}
) } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 0e2353700f..01fa3be360 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -232,19 +232,24 @@ export default class ProviderView extends View { } } - async recursivelyListAllFiles (path, files = []) { - let curPath = path + async recursivelyListAllFiles (folder, files = []) { + let curPath = folder.requestPath // need to repeat the list call until there are no more pages while (curPath) { const res = await this.provider.list(curPath) + const numSubFolders = res.items.filter((item) => item.isFolder).length + if (numSubFolders > 0) { + this.setLoading(this.plugin.uppy.i18n('traversingSubfolders', { numSubFolders, folder: folder.name })) + } + // limit concurrency (because what if we have a folder with 1000 folders inside!) // todo this might still lead to a memory run-off await pMap(res.items, async (item) => { if (item.isFolder) { // recursively call self for folder - await this.recursivelyListAllFiles(item.requestPath, files) + await this.recursivelyListAllFiles(item, files) } else { files.push(item) } @@ -268,7 +273,7 @@ export default class ProviderView extends View { for (const file of currentSelection) { if (file.isFolder) { const folder = file - const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) + const filesInFolder = await this.recursivelyListAllFiles(folder) // If the same folder is added again, we don't want to send // X amount of duplicate file notifications, we want to say @@ -369,7 +374,7 @@ export default class ProviderView extends View { if (loading) { return ( - + ) } diff --git a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx index 10af4beaa2..a6a54a39b2 100644 --- a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx +++ b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx @@ -166,7 +166,7 @@ export default class SearchProviderView extends View { if (loading) { return ( - + ) } From 85548ce2fc3688972d5145f8812f7feb45fd64af Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 14:05:40 +0900 Subject: [PATCH 11/24] run corepack yarn run build:locale-pack --- packages/@uppy/locales/src/en_US.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/locales/src/en_US.js b/packages/@uppy/locales/src/en_US.js index c29bbadfba..8a5b2d160c 100644 --- a/packages/@uppy/locales/src/en_US.js +++ b/packages/@uppy/locales/src/en_US.js @@ -164,6 +164,7 @@ en_US.strings = { takePicture: 'Take a picture', takePictureBtn: 'Take Picture', timedOut: 'Upload stalled for %{seconds} seconds, aborting.', + traversingSubfolders: 'Traversing %{numSubFolders} subfolder(s) inside "%{folder}"', upload: 'Upload', uploadComplete: 'Upload complete', uploadFailed: 'Upload failed', From 647e48eece842eea67f9f863d0d5282881bb62da Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 20:22:46 +0900 Subject: [PATCH 12/24] Revert "run corepack yarn run build:locale-pack" This reverts commit 85548ce2fc3688972d5145f8812f7feb45fd64af. --- packages/@uppy/locales/src/en_US.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@uppy/locales/src/en_US.js b/packages/@uppy/locales/src/en_US.js index 8a5b2d160c..c29bbadfba 100644 --- a/packages/@uppy/locales/src/en_US.js +++ b/packages/@uppy/locales/src/en_US.js @@ -164,7 +164,6 @@ en_US.strings = { takePicture: 'Take a picture', takePictureBtn: 'Take Picture', timedOut: 'Upload stalled for %{seconds} seconds, aborting.', - traversingSubfolders: 'Traversing %{numSubFolders} subfolder(s) inside "%{folder}"', upload: 'Upload', uploadComplete: 'Upload complete', uploadFailed: 'Upload failed', From 5434fc8e899a02a8dafa33802c7e460452cb27d9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Mar 2023 20:22:58 +0900 Subject: [PATCH 13/24] Revert "add user feedback" This reverts commit 4060019c359c529415118ca073c31b373e737f90. --- packages/@uppy/core/src/locale.js | 1 - packages/@uppy/provider-views/src/Loader.jsx | 3 +-- .../src/ProviderView/ProviderView.jsx | 15 +++++---------- .../src/SearchProviderView/SearchProviderView.jsx | 2 +- 4 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/@uppy/core/src/locale.js b/packages/@uppy/core/src/locale.js index 7c62904bb8..90bcaa9f48 100644 --- a/packages/@uppy/core/src/locale.js +++ b/packages/@uppy/core/src/locale.js @@ -49,7 +49,6 @@ export default { enterTextToSearch: 'Enter text to search for images', search: 'Search', emptyFolderAdded: 'No files were added from empty folder', - traversingSubfolders: 'Traversing %{numSubFolders} subfolder(s) inside "%{folder}"', folderAlreadyAdded: 'The folder "%{folder}" was already added', folderAdded: { 0: 'Added %{smart_count} file from %{folder}', diff --git a/packages/@uppy/provider-views/src/Loader.jsx b/packages/@uppy/provider-views/src/Loader.jsx index 57a6443663..0572ad2aae 100644 --- a/packages/@uppy/provider-views/src/Loader.jsx +++ b/packages/@uppy/provider-views/src/Loader.jsx @@ -1,10 +1,9 @@ import { h } from 'preact' -export default ({ i18n, loading }) => { +export default ({ i18n }) => { return (
{i18n('loading')} - {typeof loading === 'string' && {loading}}
) } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 01fa3be360..0e2353700f 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -232,24 +232,19 @@ export default class ProviderView extends View { } } - async recursivelyListAllFiles (folder, files = []) { - let curPath = folder.requestPath + async recursivelyListAllFiles (path, files = []) { + let curPath = path // need to repeat the list call until there are no more pages while (curPath) { const res = await this.provider.list(curPath) - const numSubFolders = res.items.filter((item) => item.isFolder).length - if (numSubFolders > 0) { - this.setLoading(this.plugin.uppy.i18n('traversingSubfolders', { numSubFolders, folder: folder.name })) - } - // limit concurrency (because what if we have a folder with 1000 folders inside!) // todo this might still lead to a memory run-off await pMap(res.items, async (item) => { if (item.isFolder) { // recursively call self for folder - await this.recursivelyListAllFiles(item, files) + await this.recursivelyListAllFiles(item.requestPath, files) } else { files.push(item) } @@ -273,7 +268,7 @@ export default class ProviderView extends View { for (const file of currentSelection) { if (file.isFolder) { const folder = file - const filesInFolder = await this.recursivelyListAllFiles(folder) + const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) // If the same folder is added again, we don't want to send // X amount of duplicate file notifications, we want to say @@ -374,7 +369,7 @@ export default class ProviderView extends View { if (loading) { return ( - + ) } diff --git a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx index a6a54a39b2..10af4beaa2 100644 --- a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx +++ b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx @@ -166,7 +166,7 @@ export default class SearchProviderView extends View { if (loading) { return ( - + ) } From 50a0bece35b0218274b9f1f6446c7cc6f843ae95 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 29 Mar 2023 18:52:06 +0200 Subject: [PATCH 14/24] use async generator instead of p-map --- packages/@uppy/provider-views/package.json | 1 - .../src/ProviderView/ProviderView.jsx | 79 ++++++++----------- yarn.lock | 43 ---------- 3 files changed, 35 insertions(+), 88 deletions(-) diff --git a/packages/@uppy/provider-views/package.json b/packages/@uppy/provider-views/package.json index cca2921d6f..ec4f4f3b0f 100644 --- a/packages/@uppy/provider-views/package.json +++ b/packages/@uppy/provider-views/package.json @@ -22,7 +22,6 @@ "dependencies": { "@uppy/utils": "workspace:^", "classnames": "^2.2.6", - "p-map": "^5.5.0", "preact": "^10.5.13" }, "peerDependencies": { diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 0e2353700f..33569ac8f2 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,5 +1,4 @@ import { h } from 'preact' -import pMap from 'p-map' import { generateFileId2 } from '@uppy/core/src/Uppy.js' @@ -232,28 +231,24 @@ export default class ProviderView extends View { } } - async recursivelyListAllFiles (path, files = []) { + async* recursivelyListAllFiles (path) { let curPath = path // need to repeat the list call until there are no more pages while (curPath) { const res = await this.provider.list(curPath) - // limit concurrency (because what if we have a folder with 1000 folders inside!) - // todo this might still lead to a memory run-off - await pMap(res.items, async (item) => { + for (const item of res.items) { if (item.isFolder) { // recursively call self for folder - await this.recursivelyListAllFiles(item.requestPath, files) + yield* this.recursivelyListAllFiles(item.requestPath) } else { - files.push(item) + yield item } - }, { concurrency: 10 }) + } curPath = res.nextPagePath } - - return files } async donePicking () { @@ -261,53 +256,49 @@ export default class ProviderView extends View { try { const { currentSelection } = this.plugin.getPluginState() - const allFiles = [] - const foldersAdded = [] + const messages = [] // eslint-disable-next-line no-unreachable-loop for (const file of currentSelection) { if (file.isFolder) { - const folder = file - const filesInFolder = await this.recursivelyListAllFiles(folder.requestPath) - - // If the same folder is added again, we don't want to send - // X amount of duplicate file notifications, we want to say - // the folder was already added. This checks if all files are duplicate, - // if that's the case, we don't add the files. - const newFilesInFolder = filesInFolder.filter((fileInFolder) => { + const { requestPath, name } = file + let isEmpty = true + let numNewFiles = 0 + for await (const fileInFolder of this.recursivelyListAllFiles(requestPath)) { const tagFile = this.getTagFile(fileInFolder) const id = generateFileId2(tagFile) - return !this.plugin.uppy.checkIfFileAlreadyExists(id) - }) + // If the same folder is added again, we don't want to send + // X amount of duplicate file notifications, we want to say + // the folder was already added. This checks if all files are duplicate, + // if that's the case, we don't add the files. + if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { + this.addFile(fileInFolder) + numNewFiles++ + } + isEmpty = false + } - allFiles.push(...newFilesInFolder) + let message + if (isEmpty) { + message = this.plugin.uppy.i18n('emptyFolderAdded') + } else if (numNewFiles === 0) { + message = this.plugin.uppy.i18n('folderAlreadyAdded', { + folder: name, + }) + } else { + message = this.plugin.uppy.i18n('folderAdded', { + smart_count: numNewFiles, folder: name, + }) + } - foldersAdded.push({ numFiles: filesInFolder.length, numNewFiles: newFilesInFolder.length, name: folder.name }) + messages.push(message) } else { - allFiles.push(file) + this.addFile(file) } } - allFiles.forEach((file) => this.addFile(file)) - this.plugin.setPluginState({ filterInput: '' }) - - for (const { name, numFiles, numNewFiles } of foldersAdded) { - let message - if (numFiles === 0) { - message = this.plugin.uppy.i18n('emptyFolderAdded') - } else if (numNewFiles === 0) { - message = this.plugin.uppy.i18n('folderAlreadyAdded', { - folder: name, - }) - } else { - message = this.plugin.uppy.i18n('folderAdded', { - smart_count: numNewFiles, folder: name, - }) - } - - this.plugin.uppy.info(message) - } + messages.forEach(message => this.plugin.uppy.info(message)) this.clearSelection() } catch (err) { diff --git a/yarn.lock b/yarn.lock index 346075496c..f2c6a8fe3b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8954,7 +8954,6 @@ __metadata: dependencies: "@uppy/utils": "workspace:^" classnames: ^2.2.6 - p-map: ^5.5.0 preact: ^10.5.13 peerDependencies: "@uppy/core": "workspace:^" @@ -10068,16 +10067,6 @@ __metadata: languageName: node linkType: hard -"aggregate-error@npm:^4.0.0": - version: 4.0.1 - resolution: "aggregate-error@npm:4.0.1" - dependencies: - clean-stack: ^4.0.0 - indent-string: ^5.0.0 - checksum: bb3ffdfd13447800fff237c2cba752c59868ee669104bb995dfbbe0b8320e967d679e683dabb640feb32e4882d60258165cde0baafc4cd467cc7d275a13ad6b5 - languageName: node - linkType: hard - "airbnb-js-shims@npm:^2.2.1": version: 2.2.1 resolution: "airbnb-js-shims@npm:2.2.1" @@ -12564,15 +12553,6 @@ __metadata: languageName: node linkType: hard -"clean-stack@npm:^4.0.0": - version: 4.2.0 - resolution: "clean-stack@npm:4.2.0" - dependencies: - escape-string-regexp: 5.0.0 - checksum: 373f656a31face5c615c0839213b9b542a0a48057abfb1df66900eab4dc2a5c6097628e4a0b5aa559cdfc4e66f8a14ea47be9681773165a44470ef5fb8ccc172 - languageName: node - linkType: hard - "cli-boxes@npm:^2.2.1": version: 2.2.1 resolution: "cli-boxes@npm:2.2.1" @@ -15866,13 +15846,6 @@ __metadata: languageName: node linkType: hard -"escape-string-regexp@npm:5.0.0": - version: 5.0.0 - resolution: "escape-string-regexp@npm:5.0.0" - checksum: 20daabe197f3cb198ec28546deebcf24b3dbb1a5a269184381b3116d12f0532e06007f4bc8da25669d6a7f8efb68db0758df4cd981f57bc5b57f521a3e12c59e - languageName: node - linkType: hard - "escape-string-regexp@npm:^1.0.5": version: 1.0.5 resolution: "escape-string-regexp@npm:1.0.5" @@ -19490,13 +19463,6 @@ __metadata: languageName: node linkType: hard -"indent-string@npm:^5.0.0": - version: 5.0.0 - resolution: "indent-string@npm:5.0.0" - checksum: e466c27b6373440e6d84fbc19e750219ce25865cb82d578e41a6053d727e5520dc5725217d6eb1cc76005a1bb1696a0f106d84ce7ebda3033b963a38583fb3b3 - languageName: node - linkType: hard - "indexes-of@npm:^1.0.1": version: 1.0.1 resolution: "indexes-of@npm:1.0.1" @@ -25839,15 +25805,6 @@ __metadata: languageName: node linkType: hard -"p-map@npm:^5.5.0": - version: 5.5.0 - resolution: "p-map@npm:5.5.0" - dependencies: - aggregate-error: ^4.0.0 - checksum: 065cb6fca6b78afbd070dd9224ff160dc23eea96e57863c09a0c8ea7ce921043f76854be7ee0abc295cff1ac9adcf700e79a1fbe3b80b625081087be58e7effb - languageName: node - linkType: hard - "p-retry@npm:^4.5.0": version: 4.6.2 resolution: "p-retry@npm:4.6.2" From f91b514be2256290b3b5053131c3a5ff7db9339d Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Mar 2023 14:35:10 +0900 Subject: [PATCH 15/24] re-fix race-condition --- .../src/ProviderView/ProviderView.jsx | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 33569ac8f2..701083bb53 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -257,6 +257,7 @@ export default class ProviderView extends View { const { currentSelection } = this.plugin.getPluginState() const messages = [] + const newFiles = [] // eslint-disable-next-line no-unreachable-loop for (const file of currentSelection) { @@ -264,6 +265,7 @@ export default class ProviderView extends View { const { requestPath, name } = file let isEmpty = true let numNewFiles = 0 + for await (const fileInFolder of this.recursivelyListAllFiles(requestPath)) { const tagFile = this.getTagFile(fileInFolder) const id = generateFileId2(tagFile) @@ -272,7 +274,7 @@ export default class ProviderView extends View { // the folder was already added. This checks if all files are duplicate, // if that's the case, we don't add the files. if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) { - this.addFile(fileInFolder) + newFiles.push(fileInFolder) numNewFiles++ } isEmpty = false @@ -293,10 +295,17 @@ export default class ProviderView extends View { messages.push(message) } else { - this.addFile(file) + newFiles.push(file) } } + // Note: this.addFile must be only run once we are done fetching all files, + // because it will cause the loading screen to disappear, + // and that will allow the user to start the upload, so we need to make sure we have + // finished all async operations before we add any file + // see https://github.com/transloadit/uppy/pull/4384 + newFiles.forEach((file) => this.addFile(file)) + this.plugin.setPluginState({ filterInput: '' }) messages.forEach(message => this.plugin.uppy.info(message)) From b5d6b78d8d7c3da368bcbca63f919f5b9ffbaa85 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Mar 2023 23:35:00 +0900 Subject: [PATCH 16/24] remove providerFileToId as suggested by @arturi --- packages/@uppy/provider-views/src/View.js | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index ff31e6ad3f..5012942586 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -1,16 +1,7 @@ import getFileType from '@uppy/utils/lib/getFileType' import isPreviewSupported from '@uppy/utils/lib/isPreviewSupported' -import generateFileID from '@uppy/utils/lib/generateFileID' import remoteFileObjToLocal from '@uppy/utils/lib/remoteFileObjToLocal' -function providerFileToId (file) { - return generateFileID({ - data: file, - name: file.name || file.id, - type: file.mimetype, - }) -} - export default class View { constructor (plugin, opts) { this.plugin = plugin @@ -68,7 +59,7 @@ export default class View { // todo document what is a "tagFile" or get rid of this concept getTagFile (file) { const tagFile = { - id: providerFileToId(file), // todo what's this used for? can be removed? + id: file.id, source: this.plugin.id, data: file, name: file.name || file.id, From fa3e66ff044e47187f6dc77e070b1e4b1f1d920a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Mar 2023 23:50:43 +0900 Subject: [PATCH 17/24] use addFiles instead of addFile --- .../src/ProviderView/ProviderView.jsx | 4 ++-- .../SearchProviderView/SearchProviderView.jsx | 3 +-- packages/@uppy/provider-views/src/View.js | 19 ++++--------------- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 297734e940..49fb9a6214 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -303,12 +303,12 @@ export default class ProviderView extends View { } } - // Note: this.addFile must be only run once we are done fetching all files, + // Note: this.addFiles must be only run once we are done fetching all files, // because it will cause the loading screen to disappear, // and that will allow the user to start the upload, so we need to make sure we have // finished all async operations before we add any file // see https://github.com/transloadit/uppy/pull/4384 - newFiles.forEach((file) => this.addFile(file)) + this.addFiles(newFiles) this.plugin.setPluginState({ filterInput: '' }) messages.forEach(message => this.plugin.uppy.info(message)) diff --git a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx index 17d96fbd43..3824e8c73e 100644 --- a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx +++ b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx @@ -36,7 +36,6 @@ export default class SearchProviderView extends View { this.search = this.search.bind(this) this.clearSearch = this.clearSearch.bind(this) this.resetPluginState = this.resetPluginState.bind(this) - this.addFile = this.addFile.bind(this) this.handleScroll = this.handleScroll.bind(this) this.donePicking = this.donePicking.bind(this) @@ -124,7 +123,7 @@ export default class SearchProviderView extends View { donePicking () { const { currentSelection } = this.plugin.getPluginState() - currentSelection.forEach((file) => this.addFile(file)) + this.addFiles(currentSelection) this.resetPluginState() } diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index 5012942586..49cd899cc8 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -11,7 +11,6 @@ export default class View { this.preFirstRender = this.preFirstRender.bind(this) this.handleError = this.handleError.bind(this) - this.addFile = this.addFile.bind(this) this.clearSelection = this.clearSelection.bind(this) this.cancelPicking = this.cancelPicking.bind(this) } @@ -95,20 +94,10 @@ export default class View { return tagFile } - addFile (file) { - const tagFile = this.getTagFile(file) - - this.plugin.uppy.log('Adding remote file') - - try { - this.plugin.uppy.addFile(tagFile) - return true - } catch (err) { - if (!err.isRestriction) { - this.plugin.uppy.log(err) - } - return false - } + addFiles (files) { + this.plugin.uppy.log('Adding remote files') + const tagFiles = files.map((file) => this.getTagFile(file)) + this.plugin.uppy.addFiles(tagFiles) } filterItems = (items) => { From 4795c4135245bc73677a35f6f103cc0c94afc9f6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 31 Mar 2023 00:06:00 +0900 Subject: [PATCH 18/24] rename function --- packages/@uppy/core/src/Uppy.js | 5 ++--- .../@uppy/provider-views/src/ProviderView/ProviderView.jsx | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index af6e7aec8c..9a0ad9341b 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -458,7 +458,7 @@ class Uppy { const fileName = getFileName(fileType, fileDescriptor) const fileExtension = getFileNameAndExtension(fileName).extension const isRemote = Boolean(fileDescriptor.isRemote) - const id = generateFileId2(fileDescriptor) + const id = generateFileIdWithType(fileDescriptor) if (this.checkIfFileAlreadyExists(id)) { const error = new RestrictionError(this.i18n('noDuplicates', { fileName })) @@ -1570,8 +1570,7 @@ class Uppy { } } -// todo come up with a better name or remove the need for two different generators -export function generateFileId2 (file) { +export function generateFileIdWithType (file) { const fileType = getFileType(file) return generateFileID({ diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 49fb9a6214..82ac2ed675 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,6 +1,6 @@ import { h } from 'preact' -import { generateFileId2 } from '@uppy/core/src/Uppy.js' +import { generateFileIdWithType } from '@uppy/core/src/Uppy.js' import AuthView from './AuthView.jsx' import Header from './Header.jsx' @@ -272,7 +272,7 @@ export default class ProviderView extends View { for await (const fileInFolder of this.recursivelyListAllFiles(requestPath)) { const tagFile = this.getTagFile(fileInFolder) - const id = generateFileId2(tagFile) + const id = generateFileIdWithType(tagFile) // If the same folder is added again, we don't want to send // X amount of duplicate file notifications, we want to say // the folder was already added. This checks if all files are duplicate, From 75ab3d59fb599270ccccccd535aa722de7477fd1 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 31 Mar 2023 00:32:18 +0900 Subject: [PATCH 19/24] use provider-supplied file ID instead of generating an ID, for providers that we whitelsit this allows adding the same time many times (with a different path) --- packages/@uppy/core/src/Uppy.js | 15 +++++++++++++-- .../src/ProviderView/ProviderView.jsx | 4 ++-- packages/@uppy/provider-views/src/View.js | 1 + 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index 9a0ad9341b..522ca0f419 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -458,7 +458,7 @@ class Uppy { const fileName = getFileName(fileType, fileDescriptor) const fileExtension = getFileNameAndExtension(fileName).extension const isRemote = Boolean(fileDescriptor.isRemote) - const id = generateFileIdWithType(fileDescriptor) + const id = getSafeFileId(fileDescriptor) if (this.checkIfFileAlreadyExists(id)) { const error = new RestrictionError(this.i18n('noDuplicates', { fileName })) @@ -1570,7 +1570,18 @@ class Uppy { } } -export function generateFileIdWithType (file) { +// If the provider has a stable, unique ID, then we can use that to identify the file. +// Then we don't have to generate our own ID, and we can add the same file many times if needed (different path) +function hasFileStableId (file) { + if (!file.isRemote || !file.remote) return false + // These are the providers that it seems like have stable IDs for their files. The other's I haven't checked yet. + const stableIdProviders = new Set(['box', 'dropbox', 'drive', 'facebook', 'unsplash']) + return stableIdProviders.has(file.remote.provider) +} + +export function getSafeFileId (file) { + if (hasFileStableId(file)) return file.id + const fileType = getFileType(file) return generateFileID({ diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 82ac2ed675..db8efdf2f8 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,6 +1,6 @@ import { h } from 'preact' -import { generateFileIdWithType } from '@uppy/core/src/Uppy.js' +import { getSafeFileId } from '@uppy/core/src/Uppy.js' import AuthView from './AuthView.jsx' import Header from './Header.jsx' @@ -272,7 +272,7 @@ export default class ProviderView extends View { for await (const fileInFolder of this.recursivelyListAllFiles(requestPath)) { const tagFile = this.getTagFile(fileInFolder) - const id = generateFileIdWithType(tagFile) + const id = getSafeFileId(tagFile) // If the same folder is added again, we don't want to send // X amount of duplicate file notifications, we want to say // the folder was already added. This checks if all files are duplicate, diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index 49cd899cc8..d9760e9d61 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -76,6 +76,7 @@ export default class View { }, providerOptions: this.provider.opts, providerName: this.provider.name, + provider: this.provider.provider, }, } From 79abf68ccc8f9fd31e87495a26a06c5498733c38 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 31 Mar 2023 00:36:53 +0900 Subject: [PATCH 20/24] call core directly --- .../@uppy/provider-views/src/ProviderView/ProviderView.jsx | 5 +++-- .../src/SearchProviderView/SearchProviderView.jsx | 3 ++- packages/@uppy/provider-views/src/View.js | 6 ------ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index db8efdf2f8..ad1148c4b3 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -303,12 +303,13 @@ export default class ProviderView extends View { } } - // Note: this.addFiles must be only run once we are done fetching all files, + // Note: this.plugin.uppy.addFiles must be only run once we are done fetching all files, // because it will cause the loading screen to disappear, // and that will allow the user to start the upload, so we need to make sure we have // finished all async operations before we add any file // see https://github.com/transloadit/uppy/pull/4384 - this.addFiles(newFiles) + this.plugin.uppy.log('Adding remote provider files') + this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file))) this.plugin.setPluginState({ filterInput: '' }) messages.forEach(message => this.plugin.uppy.info(message)) diff --git a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx index 3824e8c73e..c2bde793f7 100644 --- a/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx +++ b/packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx @@ -123,7 +123,8 @@ export default class SearchProviderView extends View { donePicking () { const { currentSelection } = this.plugin.getPluginState() - this.addFiles(currentSelection) + this.plugin.uppy.log('Adding remote search provider files') + this.plugin.uppy.addFiles(currentSelection.map((file) => this.getTagFile(file))) this.resetPluginState() } diff --git a/packages/@uppy/provider-views/src/View.js b/packages/@uppy/provider-views/src/View.js index d9760e9d61..910a2ed8a3 100644 --- a/packages/@uppy/provider-views/src/View.js +++ b/packages/@uppy/provider-views/src/View.js @@ -95,12 +95,6 @@ export default class View { return tagFile } - addFiles (files) { - this.plugin.uppy.log('Adding remote files') - const tagFiles = files.map((file) => this.getTagFile(file)) - this.plugin.uppy.addFiles(tagFiles) - } - filterItems = (items) => { const state = this.plugin.getPluginState() if (!state.filterInput || state.filterInput === '') { From cf01707962028263e3c60cff0e265a897fc5a2e6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 31 Mar 2023 01:02:14 +0900 Subject: [PATCH 21/24] improve dev dashboard --- private/dev/Dashboard.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/private/dev/Dashboard.js b/private/dev/Dashboard.js index a1e5ad227f..0e98db614a 100644 --- a/private/dev/Dashboard.js +++ b/private/dev/Dashboard.js @@ -55,6 +55,9 @@ async function assemblyOptions () { // Rest is implementation! Obviously edit as necessary... export default () => { + const restrictions = undefined + // const restrictions = { requiredMetaFields: ['caption'], maxNumberOfFiles: 3 } + const uppyDashboard = new Uppy({ logger: debugLogger, meta: { @@ -62,7 +65,7 @@ export default () => { license: 'Creative Commons', }, allowMultipleUploadBatches: false, - // restrictions: { requiredMetaFields: ['caption'] }, + restrictions, }) .use(Dashboard, { trigger: '#pick-files', @@ -74,7 +77,7 @@ export default () => { ], showProgressDetails: true, proudlyDisplayPoweredByUppy: true, - note: '2 files, images and video only', + note: `${JSON.stringify(restrictions)}`, }) // .use(GoogleDrive, { target: Dashboard, companionUrl: COMPANION_URL, companionAllowedHosts }) // .use(Instagram, { target: Dashboard, companionUrl: COMPANION_URL, companionAllowedHosts }) From d1d913221ba56ec750999f4dcd5ba71c4a4f94ea Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sat, 1 Apr 2023 00:05:21 +0900 Subject: [PATCH 22/24] disable experimental getAsFileSystemHandle it seems to be causing problems when dropping folders with subfolders in newest chrome e.g a folder with 50 files and a subfolder which in turn has another 50 files also refactor and document the code more --- .../utils/webkitGetAsEntryApi/index.js | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/index.js b/packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/index.js index f89ba18c69..08ee4fd3b8 100644 --- a/packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/index.js +++ b/packages/@uppy/utils/src/getDroppedFiles/utils/webkitGetAsEntryApi/index.js @@ -1,7 +1,8 @@ import getFilesAndDirectoriesFromDirectory from './getFilesAndDirectoriesFromDirectory.js' /** - * Interop between deprecated webkitGetAsEntry and standard getAsFileSystemHandle. + * Polyfill for the new (experimental) getAsFileSystemHandle API (using the popular webkitGetAsEntry behind the scenes) + * so that we can switch to the getAsFileSystemHandle API once it (hopefully) becomes standard */ function getAsFileSystemHandleFromEntry (entry, logDropError) { if (entry == null) return entry @@ -29,36 +30,59 @@ async function* createPromiseToAddFileOrParseDirectory (entry, relativePath, las // For each dropped item, - make sure it's a file/directory, and start deepening in! if (entry.kind === 'file') { const file = await entry.getFile() - if (file !== null) { + if (file != null) { file.relativePath = relativePath ? `${relativePath}/${entry.name}` : null yield file } else if (lastResortFile != null) yield lastResortFile } else if (entry.kind === 'directory') { for await (const handle of entry.values()) { + // Recurse on the directory, appending the dir name to the relative path yield* createPromiseToAddFileOrParseDirectory(handle, `${relativePath}/${entry.name}`) } } else if (lastResortFile != null) yield lastResortFile } +/** + * Load all files from data transfer, and recursively read any directories. + * Note that IE is not supported for drag-drop, because IE doesn't support Data Transfers + * + * @param {DataTransfer} dataTransfer + * @param {*} logDropError on error + */ export default async function* getFilesFromDataTransfer (dataTransfer, logDropError) { - const entries = await Promise.all(Array.from(dataTransfer.items, async item => { - const lastResortFile = item.getAsFile() // Chromium bug, see https://github.com/transloadit/uppy/issues/3505. - let entry - // IMPORTANT: Need to check isSecureContext *first* or else Chrome will crash when running in HTTP: - // https://github.com/transloadit/uppy/issues/4133 - if (window.isSecureContext && item.getAsFileSystemHandle != null) entry = await item.getAsFileSystemHandle() - // fallback - entry ??= getAsFileSystemHandleFromEntry(item.webkitGetAsEntry(), logDropError) + // Retrieving the dropped items must happen synchronously + // otherwise only the first item gets treated and the other ones are garbage collected. + // https://github.com/transloadit/uppy/pull/3998 + const fileSystemHandles = await Promise.all(Array.from(dataTransfer.items, async item => { + let fileSystemHandle + + // TODO enable getAsFileSystemHandle API once we can get it working with subdirectories + // IMPORTANT: Need to check isSecureContext *before* calling getAsFileSystemHandle + // or else Chrome will crash when running in HTTP: https://github.com/transloadit/uppy/issues/4133 + // if (window.isSecureContext && item.getAsFileSystemHandle != null) entry = await item.getAsFileSystemHandle() - return { lastResortFile, entry } + // `webkitGetAsEntry` exists in all popular browsers (including non-WebKit browsers), + // however it may be renamed to getAsEntry() in the future, so you should code defensively, looking for both. + // from https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/webkitGetAsEntry + const getAsEntry = () => (typeof item.getAsEntry === 'function' ? item.getAsEntry() : item.webkitGetAsEntry()) + // eslint-disable-next-line prefer-const + fileSystemHandle ??= getAsFileSystemHandleFromEntry(getAsEntry(), logDropError) + + return { + fileSystemHandle, + lastResortFile: item.getAsFile(), // can be used as a fallback in case other methods fail + } })) - for (const { lastResortFile, entry } of entries) { - // :entry can be null when we drop the url e.g. - if (entry != null) { + for (const { lastResortFile, fileSystemHandle } of fileSystemHandles) { + // fileSystemHandle and lastResortFile can be null when we drop an url. + if (fileSystemHandle != null) { try { - yield* createPromiseToAddFileOrParseDirectory(entry, '', lastResortFile) + yield* createPromiseToAddFileOrParseDirectory(fileSystemHandle, '', lastResortFile) } catch (err) { + // Example: If dropping a symbolic link, Chromium will throw: + // "DOMException: A requested file or directory could not be found at the time an operation was processed.", + // So we will use lastResortFile instead. See https://github.com/transloadit/uppy/issues/3505. if (lastResortFile != null) { yield lastResortFile } else { From 8f5c32dea9a3da6ebb86738bd8757269bb193093 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sat, 1 Apr 2023 11:22:16 +0900 Subject: [PATCH 23/24] Update packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx Co-authored-by: Antoine du Hamel --- packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index ad1148c4b3..a0008a6d7a 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,6 +1,6 @@ import { h } from 'preact' -import { getSafeFileId } from '@uppy/core/src/Uppy.js' +import { getSafeFileId } from '@uppy/core/lib/Uppy.js' import AuthView from './AuthView.jsx' import Header from './Header.jsx' From 46368c8db1a2e4430d05acdd7c07ce1eca6d16da Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sat, 1 Apr 2023 14:41:57 +0900 Subject: [PATCH 24/24] mov eto utils --- packages/@uppy/core/src/Uppy.js | 22 +------------------ .../src/ProviderView/ProviderView.jsx | 2 +- packages/@uppy/utils/src/generateFileID.js | 22 +++++++++++++++++++ 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index 522ca0f419..ce45394923 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -8,7 +8,7 @@ import throttle from 'lodash.throttle' import DefaultStore from '@uppy/store-default' import getFileType from '@uppy/utils/lib/getFileType' import getFileNameAndExtension from '@uppy/utils/lib/getFileNameAndExtension' -import generateFileID from '@uppy/utils/lib/generateFileID' +import { getSafeFileId } from '@uppy/utils/lib/generateFileID' import supportsUploadProgress from './supportsUploadProgress.js' import getFileName from './getFileName.js' import { justErrorsLogger, debugLogger } from './loggers.js' @@ -1570,24 +1570,4 @@ class Uppy { } } -// If the provider has a stable, unique ID, then we can use that to identify the file. -// Then we don't have to generate our own ID, and we can add the same file many times if needed (different path) -function hasFileStableId (file) { - if (!file.isRemote || !file.remote) return false - // These are the providers that it seems like have stable IDs for their files. The other's I haven't checked yet. - const stableIdProviders = new Set(['box', 'dropbox', 'drive', 'facebook', 'unsplash']) - return stableIdProviders.has(file.remote.provider) -} - -export function getSafeFileId (file) { - if (hasFileStableId(file)) return file.id - - const fileType = getFileType(file) - - return generateFileID({ - ...file, - type: fileType, - }) -} - export default Uppy diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index a0008a6d7a..3690cf6348 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -1,6 +1,6 @@ import { h } from 'preact' -import { getSafeFileId } from '@uppy/core/lib/Uppy.js' +import { getSafeFileId } from '@uppy/utils/lib/generateFileID' import AuthView from './AuthView.jsx' import Header from './Header.jsx' diff --git a/packages/@uppy/utils/src/generateFileID.js b/packages/@uppy/utils/src/generateFileID.js index 73a88fc4e9..bdb6367658 100644 --- a/packages/@uppy/utils/src/generateFileID.js +++ b/packages/@uppy/utils/src/generateFileID.js @@ -1,3 +1,5 @@ +import getFileType from './getFileType.js' + function encodeCharacter (character) { return character.charCodeAt(0).toString(32) } @@ -43,3 +45,23 @@ export default function generateFileID (file) { return id } + +// If the provider has a stable, unique ID, then we can use that to identify the file. +// Then we don't have to generate our own ID, and we can add the same file many times if needed (different path) +function hasFileStableId (file) { + if (!file.isRemote || !file.remote) return false + // These are the providers that it seems like have stable IDs for their files. The other's I haven't checked yet. + const stableIdProviders = new Set(['box', 'dropbox', 'drive', 'facebook', 'unsplash']) + return stableIdProviders.has(file.remote.provider) +} + +export function getSafeFileId (file) { + if (hasFileStableId(file)) return file.id + + const fileType = getFileType(file) + + return generateFileID({ + ...file, + type: fileType, + }) +}