Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
Apr 15, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions packages/@uppy/core/src/Restricter.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,37 @@ class Restricter {
}
}

validate (file, files) {
const { maxFileSize, minFileSize, maxTotalFileSize, maxNumberOfFiles, allowedFileTypes } = this.getOpts().restrictions
validateTotals (existingFiles, newFiles) {
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
const { maxTotalFileSize, maxNumberOfFiles } = this.getOpts().restrictions

if (maxNumberOfFiles) {
const nonGhostFiles = files.filter(f => !f.isGhost)
if (nonGhostFiles.length + 1 > maxNumberOfFiles) {
const nonGhostFiles = existingFiles.filter(f => !f.isGhost)
if (nonGhostFiles.length + newFiles.length > maxNumberOfFiles) {
throw new RestrictionError(`${this.i18n('youCanOnlyUploadX', { smart_count: maxNumberOfFiles })}`)
}
}

if (maxTotalFileSize) {
let totalFilesSize = existingFiles.reduce((total, f) => (total + f.size), 0)

for (const newFile of newFiles) {
if (newFile.size != null) { // We can't check maxTotalFileSize if the size is unknown.
totalFilesSize += newFile.size

if (totalFilesSize > maxTotalFileSize) {
throw new RestrictionError(this.i18n('exceedsSize', {
size: prettierBytes(maxTotalFileSize),
file: newFile.name,
}))
}
}
}
}
}

validate (file) {
const { maxFileSize, minFileSize, allowedFileTypes } = this.getOpts().restrictions

if (allowedFileTypes) {
const isCorrectFileType = allowedFileTypes.some((type) => {
// check if this is a mime-type
Expand All @@ -61,18 +82,6 @@ class Restricter {
}
}

// We can't check maxTotalFileSize if the size is unknown.
if (maxTotalFileSize && file.size != null) {
const totalFilesSize = files.reduce((total, f) => (total + f.size), file.size)

if (totalFilesSize > maxTotalFileSize) {
throw new RestrictionError(this.i18n('exceedsSize', {
size: prettierBytes(maxTotalFileSize),
file: file.name,
}))
}
}

// We can't check maxFileSize if the size is unknown.
if (maxFileSize && file.size != null && file.size > maxFileSize) {
throw new RestrictionError(this.i18n('exceedsSize', {
Expand Down
90 changes: 48 additions & 42 deletions packages/@uppy/core/src/Uppy.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ class Uppy {

validateRestrictions (file, files = this.getFiles()) {
try {
this.#restricter.validate(file, files)
this.#restricter.validate(file)
this.#restricter.validateTotals(files, [file])
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
} catch (err) {
return err
}
Expand Down Expand Up @@ -508,12 +509,17 @@ class Uppy {
newFile = onBeforeFileAddedResult
}

try {
const filesArray = Object.keys(files).map(i => files[i])
this.#restricter.validate(newFile, filesArray)
} catch (err) {
this.#informAndEmit(err, newFile)
throw err
this.#restricter.validate(newFile)

// Users are asked to re-select recovered files without data,
// and to keep the progress, meta and everthing else, we only replace said data
if (files[newFile.id] && files[newFile.id].isGhost) {
mifi marked this conversation as resolved.
Show resolved Hide resolved
newFile = {
...files[newFile.id],
data: fileDescriptor.data,
isGhost: false,
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
}
this.log(`Replaced the blob in the restored ghost file: ${newFile.name}, ${newFile.id}`)
}

return newFile
Expand Down Expand Up @@ -545,17 +551,15 @@ class Uppy {
this.#assertNewUploadAllowed(file)

const { files } = this.getState()
let newFile = this.#checkAndCreateFileStateObject(files, file)

// Users are asked to re-select recovered files without data,
// and to keep the progress, meta and everthing else, we only replace said data
if (files[newFile.id] && files[newFile.id].isGhost) {
newFile = {
...files[newFile.id],
data: file.data,
isGhost: false,
}
this.log(`Replaced the blob in the restored ghost file: ${newFile.name}, ${newFile.id}`)
let newFile

try {
newFile = this.#checkAndCreateFileStateObject(files, file)
this.#restricter.validateTotals(Object.values(files), [newFile])
} catch (err) {
this.#informAndEmit(err, file)
throw err
}

this.setState({
Expand Down Expand Up @@ -589,45 +593,47 @@ class Uppy {
const newFiles = []
const errors = []
for (let i = 0; i < fileDescriptors.length; i++) {
const fileDescriptor = fileDescriptors[i]
try {
let newFile = this.#checkAndCreateFileStateObject(files, fileDescriptors[i])
// Users are asked to re-select recovered files without data,
// and to keep the progress, meta and everthing else, we only replace said data
if (files[newFile.id] && files[newFile.id].isGhost) {
newFile = {
...files[newFile.id],
data: fileDescriptors[i].data,
isGhost: false,
}
this.log(`Replaced blob in a ghost file: ${newFile.name}, ${newFile.id}`)
}
const newFile = this.#checkAndCreateFileStateObject(files, fileDescriptor)

files[newFile.id] = newFile
newFiles.push(newFile)
} catch (err) {
this.#informAndEmit(err, fileDescriptor)
if (!err.isRestriction) {
errors.push(err)
}
}
}

this.setState({ files })

newFiles.forEach((newFile) => {
this.emit('file-added', newFile)
})
try {
this.#restricter.validateTotals(Object.values(this.getState().files), newFiles)

this.emit('files-added', newFiles)
this.setState({ files })

if (newFiles.length > 5) {
this.log(`Added batch of ${newFiles.length} files`)
} else {
Object.keys(newFiles).forEach(fileID => {
this.log(`Added file: ${newFiles[fileID].name}\n id: ${newFiles[fileID].id}\n type: ${newFiles[fileID].type}`)
newFiles.forEach((newFile) => {
this.emit('file-added', newFile)
})
}

if (newFiles.length > 0) {
this.#startIfAutoProceed()
this.emit('files-added', newFiles)

if (newFiles.length > 5) {
this.log(`Added batch of ${newFiles.length} files`)
} else {
Object.keys(newFiles).forEach(fileID => {
this.log(`Added file: ${newFiles[fileID].name}\n id: ${newFiles[fileID].id}\n type: ${newFiles[fileID].type}`)
})
}

if (newFiles.length > 0) {
this.#startIfAutoProceed()
}
} catch (err) {
this.#informAndEmit(err, fileDescriptors[0])
if (!err.isRestriction) {
errors.push(err)
}
}

if (errors.length > 0) {
Expand Down
1 change: 1 addition & 0 deletions packages/@uppy/core/src/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export default {
search: 'Search',
resetSearch: 'Reset search',
emptyFolderAdded: 'No files were added from empty folder',
addedNumFiles: 'Added %{numFiles} file(s)',
folderAlreadyAdded: 'The folder "%{folder}" was already added',
folderAdded: {
0: 'Added %{smart_count} file from %{folder}',
Expand Down
24 changes: 22 additions & 2 deletions packages/@uppy/dashboard/src/components/Dashboard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,28 @@ export default function Dashboard (props) {

{showFileList ? (
<FileList
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
id={props.id}
error={props.error}
i18n={props.i18n}
uppy={props.uppy}
files={props.files}
acquirers={props.acquirers}
resumableUploads={props.resumableUploads}
hideRetryButton={props.hideRetryButton}
hidePauseResumeButton={props.hidePauseResumeButton}
hideCancelButton={props.hideCancelButton}
showLinkToFileUploadResult={props.showLinkToFileUploadResult}
showRemoveButtonAfterComplete={props.showRemoveButtonAfterComplete}
isWide={props.isWide}
metaFields={props.metaFields}
toggleFileCard={props.toggleFileCard}
handleRequestThumbnail={props.handleRequestThumbnail}
handleCancelThumbnail={props.handleCancelThumbnail}
recoveredState={props.recoveredState}
individualCancellation={props.individualCancellation}
openFileEditor={props.openFileEditor}
canEditFile={props.canEditFile}
toggleAddFilesPanel={props.toggleAddFilesPanel}
singleFile={singleFile}
itemsPerRow={itemsPerRow}
/>
Expand Down
87 changes: 44 additions & 43 deletions packages/@uppy/dashboard/src/components/FileList.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { h } from 'preact'
import { useMemo } from 'preact/hooks'
import FileItem from './FileItem/index.jsx'
import VirtualList from './VirtualList.jsx'

Expand All @@ -17,50 +18,28 @@ function chunks (list, size) {
return chunked
}

export default (props) => {
export default ({
id, error, i18n, uppy, files, acquirers, resumableUploads, hideRetryButton, hidePauseResumeButton, hideCancelButton,
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
showLinkToFileUploadResult, showRemoveButtonAfterComplete, isWide, metaFields, singleFile, toggleFileCard,
handleRequestThumbnail, handleCancelThumbnail, recoveredState, individualCancellation, itemsPerRow, openFileEditor,
canEditFile, toggleAddFilesPanel,
}) => {
// It's not great that this is hardcoded!
// It's ESPECIALLY not great that this is checking against `itemsPerRow`!
const rowHeight = props.itemsPerRow === 1
const rowHeight = itemsPerRow === 1
// Mobile
? 71
// 190px height + 2 * 5px margin
: 200

const fileProps = {
// FIXME This is confusing, it's actually the Dashboard's plugin ID
id: props.id,
error: props.error,
// TODO move this to context
i18n: props.i18n,
uppy: props.uppy,
// features
acquirers: props.acquirers,
resumableUploads: props.resumableUploads,
individualCancellation: props.individualCancellation,
// visual options
hideRetryButton: props.hideRetryButton,
hidePauseResumeButton: props.hidePauseResumeButton,
hideCancelButton: props.hideCancelButton,
showLinkToFileUploadResult: props.showLinkToFileUploadResult,
showRemoveButtonAfterComplete: props.showRemoveButtonAfterComplete,
isWide: props.isWide,
metaFields: props.metaFields,
recoveredState: props.recoveredState,
singleFile: props.singleFile,
// callbacks
toggleFileCard: props.toggleFileCard,
handleRequestThumbnail: props.handleRequestThumbnail,
handleCancelThumbnail: props.handleCancelThumbnail,
}

const sortByGhostComesFirst = (file1, file2) => {
return props.files[file2].isGhost - props.files[file1].isGhost
}

// Sort files by file.isGhost, ghost files first, only if recoveredState is present
const files = Object.keys(props.files)
if (props.recoveredState) files.sort(sortByGhostComesFirst)
const rows = chunks(files, props.itemsPerRow)
const rows = useMemo(() => {
const sortByGhostComesFirst = (file1, file2) => files[file2].isGhost - files[file1].isGhost

const fileIds = Object.keys(files)
if (recoveredState) fileIds.sort(sortByGhostComesFirst)
return chunks(fileIds, itemsPerRow)
}, [files, itemsPerRow, recoveredState])

const renderRow = (row) => (
// The `role="presentation` attribute ensures that the list items are properly
Expand All @@ -70,19 +49,41 @@ export default (props) => {
{row.map((fileID) => (
<FileItem
key={fileID}
uppy={props.uppy}
{...fileProps} // eslint-disable-line react/jsx-props-no-spreading
uppy={uppy}
// FIXME This is confusing, it's actually the Dashboard's plugin ID
id={id}
error={error}
// TODO move this to context
i18n={i18n}
// features
acquirers={acquirers}
resumableUploads={resumableUploads}
individualCancellation={individualCancellation}
// visual options
hideRetryButton={hideRetryButton}
hidePauseResumeButton={hidePauseResumeButton}
hideCancelButton={hideCancelButton}
showLinkToFileUploadResult={showLinkToFileUploadResult}
showRemoveButtonAfterComplete={showRemoveButtonAfterComplete}
isWide={isWide}
metaFields={metaFields}
recoveredState={recoveredState}
singleFile={singleFile}
// callbacks
toggleFileCard={toggleFileCard}
handleRequestThumbnail={handleRequestThumbnail}
handleCancelThumbnail={handleCancelThumbnail}
role="listitem"
openFileEditor={props.openFileEditor}
canEditFile={props.canEditFile}
toggleAddFilesPanel={props.toggleAddFilesPanel}
file={props.files[fileID]}
openFileEditor={openFileEditor}
canEditFile={canEditFile}
toggleAddFilesPanel={toggleAddFilesPanel}
file={files[fileID]}
/>
))}
</div>
)

if (props.singleFile) {
if (singleFile) {
return (
<div class="uppy-Dashboard-files">
{renderRow(rows[0])}
Expand Down
1 change: 1 addition & 0 deletions packages/@uppy/locales/src/en_US.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ en_US.strings = {
'0': 'Failed to add %{smart_count} file due to an internal error',
'1': 'Failed to add %{smart_count} files due to internal errors',
},
addedNumFiles: 'Added %{numFiles} file(s)',
addingMoreFiles: 'Adding more files',
addMore: 'Add more',
addMoreFiles: 'Add more files',
Expand Down
3 changes: 2 additions & 1 deletion packages/@uppy/locales/src/fr_FR.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ fr_FR.strings = {
'0': 'L\'ajout de %{smart_count} fichier a échoué',
'1': 'L\'ajout de %{smart_count} fichiers a échoué',
},
addedNumFiles: '%{numFiles} fichier(s) ajouté(s)',
addingMoreFiles: 'En train d\'ajouter des fichiers',
addMore: 'Ajouter d\'autres',
addMoreFiles: 'Ajouter d\'autres fichiers',
addingMoreFiles: 'En train d\'ajouter des fichiers',
allFilesFromFolderNamed: 'Tous les fichiers du dossier %{name}',
allowAccessDescription: 'Pour prendre des photos ou enregistrer une vidéo avec votre caméra, veuillez autoriser l\'accès à votre caméra pour ce site.',
allowAccessTitle: 'Veuillez autoriser l\'accès à votre caméra',
Expand Down
1 change: 1 addition & 0 deletions packages/@uppy/provider-views/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"@uppy/utils": "workspace:^",
"classnames": "^2.2.6",
"nanoid": "^4.0.0",
"p-queue": "^7.3.4",
"preact": "^10.5.13"
},
"peerDependencies": {
Expand Down
5 changes: 4 additions & 1 deletion packages/@uppy/provider-views/src/Loader.jsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { h } from 'preact'

export default ({ i18n }) => {
export default ({ i18n, loading }) => {
return (
<div className="uppy-Provider-loading">
<span>{i18n('loading')}</span>
{typeof loading === 'string' && (
<span style={{ marginTop: '.7em' }}>{loading}</span>
)}
</div>
)
}
Loading