From 013f4eab2b46ae7c105ea28f6ace7c8647410740 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 15 Aug 2024 20:10:01 +0200 Subject: [PATCH] @uppy/transloadit: fix issue with `allowMultipleUploadBatches` (#5400) * fix issue with allowMultipleUploadBatches fixes #5397 also refactor from promise.then to async/await and fix what seems like broken logic with recursive this.#afterUpload call * throw better error when all files have been canceled after an assembly has been created also rewrite #createAssembly to async/await * wait for updateAssembly when restoring fixes potential race condition --- .../integration/dashboard-transloadit.spec.ts | 6 +- packages/@uppy/transloadit/src/index.ts | 199 +++++++++--------- 2 files changed, 106 insertions(+), 99 deletions(-) diff --git a/e2e/cypress/integration/dashboard-transloadit.spec.ts b/e2e/cypress/integration/dashboard-transloadit.spec.ts index 311d42b116..faee0a33dc 100644 --- a/e2e/cypress/integration/dashboard-transloadit.spec.ts +++ b/e2e/cypress/integration/dashboard-transloadit.spec.ts @@ -65,14 +65,14 @@ describe('Dashboard with Transloadit', () => { cy.get('.uppy-StatusBar-actionBtn--upload').click() cy.wait(['@createAssemblies', '@tusCreate']).then(() => { - const plugin = getPlugin(uppy) + const { assembly } = getPlugin(uppy) - expect(plugin.assembly.closed).to.be.false + expect(assembly.closed).to.be.false uppy.cancelAll() cy.wait(['@delete', '@tusDelete']).then(() => { - expect(plugin.assembly.closed).to.be.true + expect(assembly.closed).to.be.true }) }) }) diff --git a/packages/@uppy/transloadit/src/index.ts b/packages/@uppy/transloadit/src/index.ts index a92f51a84a..dad3f498ab 100644 --- a/packages/@uppy/transloadit/src/index.ts +++ b/packages/@uppy/transloadit/src/index.ts @@ -400,64 +400,64 @@ export default class Transloadit< return newFile } - #createAssembly( + async #createAssembly( fileIDs: string[], assemblyOptions: OptionsWithRestructuredFields, ) { this.uppy.log('[Transloadit] Create Assembly') - return this.client - .createAssembly({ + try { + const newAssembly = await this.client.createAssembly({ ...assemblyOptions, expectedFiles: fileIDs.length, }) - .then(async (newAssembly) => { - const files = this.uppy - .getFiles() - .filter(({ id }) => fileIDs.includes(id)) - if (files.length === 0) { - // All files have been removed, cancelling. - await this.client.cancelAssembly(newAssembly) - return null - } - - const assembly = new Assembly(newAssembly, this.#rateLimitedQueue) - const { status } = assembly - const assemblyID = status.assembly_id - const updatedFiles: Record> = {} - files.forEach((file) => { - updatedFiles[file.id] = this.#attachAssemblyMetadata(file, status) - }) + const files = this.uppy + .getFiles() + .filter(({ id }) => fileIDs.includes(id)) - this.uppy.setState({ - files: { - ...this.uppy.getState().files, - ...updatedFiles, - }, - }) + if (files.length === 0) { + // All files have been removed, cancelling. + await this.client.cancelAssembly(newAssembly) + return null + } - this.uppy.emit('transloadit:assembly-created', status, fileIDs) + const assembly = new Assembly(newAssembly, this.#rateLimitedQueue) + const { status } = assembly + const assemblyID = status.assembly_id - this.uppy.log(`[Transloadit] Created Assembly ${assemblyID}`) - return assembly + const updatedFiles: Record> = {} + files.forEach((file) => { + updatedFiles[file.id] = this.#attachAssemblyMetadata(file, status) }) - .catch((err) => { - // TODO: use AssemblyError? - const wrapped = new ErrorWithCause( - `${this.i18n('creatingAssemblyFailed')}: ${err.message}`, - { cause: err }, - ) - if ('details' in err) { - // @ts-expect-error details is not in the Error type - wrapped.details = err.details - } - if ('assembly' in err) { - // @ts-expect-error assembly is not in the Error type - wrapped.assembly = err.assembly - } - throw wrapped + + this.uppy.setState({ + files: { + ...this.uppy.getState().files, + ...updatedFiles, + }, }) + + this.uppy.emit('transloadit:assembly-created', status, fileIDs) + + this.uppy.log(`[Transloadit] Created Assembly ${assemblyID}`) + return assembly + } catch (err) { + // TODO: use AssemblyError? + const wrapped = new ErrorWithCause( + `${this.i18n('creatingAssemblyFailed')}: ${err.message}`, + { cause: err }, + ) + if ('details' in err) { + // @ts-expect-error details is not in the Error type + wrapped.details = err.details + } + if ('assembly' in err) { + // @ts-expect-error assembly is not in the Error type + wrapped.assembly = err.assembly + } + throw wrapped + } } #createAssemblyWatcher(idOrArrayOfIds: string | string[]) { @@ -616,6 +616,7 @@ export default class Transloadit< await this.client.cancelAssembly(assembly) // TODO bubble this through AssemblyWatcher so its event handlers can clean up correctly this.uppy.emit('transloadit:assembly-cancelled', assembly) + this.assembly = undefined } /** @@ -702,20 +703,21 @@ export default class Transloadit< this.#connectAssembly(this.assembly!) } - // Force-update all Assemblies to check for missed events. - const updateAssemblies = () => { + // Force-update Assembly to check for missed events. + const updateAssembly = () => { return this.assembly?.update() } // Restore all Assembly state. - this.restored = Promise.resolve().then(() => { + this.restored = (async () => { restoreState() restoreAssemblies() - updateAssemblies() - }) - - this.restored.then(() => { + await updateAssembly() this.restored = null + })() + + this.restored.catch((err) => { + this.uppy.log('Failed to restore', err) }) } @@ -800,8 +802,11 @@ export default class Transloadit< try { const assembly = // this.assembly can already be defined if we recovered files with Golden Retriever (this.#onRestored) - (this.assembly ?? - (await this.#createAssembly(fileIDs, assemblyOptions)))! + this.assembly ?? (await this.#createAssembly(fileIDs, assemblyOptions)) + + if (assembly == null) + throw new Error('All files were canceled after assembly was created') + if (this.opts.importFromUploadURLs) { await this.#reserveFiles(assembly, fileIDs) } @@ -823,57 +828,54 @@ export default class Transloadit< } } - #afterUpload = (fileIDs: string[], uploadID: string): Promise => { - const files = fileIDs.map((fileID) => this.uppy.getFile(fileID)) - // Only use files without errors - const filteredFileIDs = files - .filter((file) => !file.error) - .map((file) => file.id) + #afterUpload = async (fileIDs: string[], uploadID: string): Promise => { + try { + // If we're still restoring state, wait for that to be done. + await this.restored - // If we're still restoring state, wait for that to be done. - if (this.restored) { - return this.restored.then(() => { - return this.#afterUpload(filteredFileIDs, uploadID) - }) - } + const files = fileIDs + .map((fileID) => this.uppy.getFile(fileID)) + // Only use files without errors + .filter((file) => !file.error) - const assemblyID = this.assembly?.status.assembly_id + const assemblyID = this.assembly?.status.assembly_id - const closeSocketConnections = () => { - this.assembly?.close() - } + const closeSocketConnections = () => { + this.assembly?.close() + } - // If we don't have to wait for encoding metadata or results, we can close - // the socket immediately and finish the upload. - if (!this.#shouldWaitAfterUpload()) { - closeSocketConnections() - const status = this.assembly?.status - if (status != null) { - this.uppy.addResultData(uploadID, { - transloadit: [status], - }) + // If we don't have to wait for encoding metadata or results, we can close + // the socket immediately and finish the upload. + if (!this.#shouldWaitAfterUpload()) { + closeSocketConnections() + const status = this.assembly?.status + if (status != null) { + this.uppy.addResultData(uploadID, { + transloadit: [status], + }) + } + return } - return Promise.resolve() - } - // If no Assemblies were created for this upload, we also do not have to wait. - // There's also no sockets or anything to close, so just return immediately. - if (!assemblyID) { - this.uppy.addResultData(uploadID, { transloadit: [] }) - return Promise.resolve() - } + // If no Assemblies were created for this upload, we also do not have to wait. + // There's also no sockets or anything to close, so just return immediately. + if (!assemblyID) { + this.uppy.addResultData(uploadID, { transloadit: [] }) + return + } - const incompleteFiles = files.filter( - (file) => !hasProperty(this.completedFiles, file.id), - ) - incompleteFiles.forEach((file) => { - this.uppy.emit('postprocess-progress', file, { - mode: 'indeterminate', - message: this.i18n('encoding'), + const incompleteFiles = files.filter( + (file) => !hasProperty(this.completedFiles, file.id), + ) + incompleteFiles.forEach((file) => { + this.uppy.emit('postprocess-progress', file, { + mode: 'indeterminate', + message: this.i18n('encoding'), + }) }) - }) - return this.#watcher.promise.then(() => { + await this.#watcher.promise + // assembly is now done processing! closeSocketConnections() const status = this.assembly?.status if (status != null) { @@ -881,7 +883,12 @@ export default class Transloadit< transloadit: [status], }) } - }) + } finally { + // in case allowMultipleUploadBatches is true and the user wants to upload again, + // we need to allow a new assembly to be created. + // see https://github.com/transloadit/uppy/issues/5397 + this.assembly = undefined + } } #closeAssemblyIfExists = () => {