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

fix issue with allowMultipleUploadBatches #5400

Merged
merged 6 commits into from
Aug 15, 2024

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Aug 9, 2024

Note: don't merge this before #5399 (or recreate this pr directly onto main)

Copy link
Contributor

github-actions bot commented Aug 9, 2024

Diff output files
diff --git a/packages/@uppy/transloadit/lib/index.js b/packages/@uppy/transloadit/lib/index.js
index 58d2664..44cb86d 100644
--- a/packages/@uppy/transloadit/lib/index.js
+++ b/packages/@uppy/transloadit/lib/index.js
@@ -223,17 +223,18 @@ export default class Transloadit extends BasePlugin {
           );
           _classPrivateFieldLooseBase(this, _connectAssembly)[_connectAssembly](this.assembly);
         };
-        const updateAssemblies = () => {
+        const updateAssembly = () => {
           var _this$assembly;
           return (_this$assembly = this.assembly) == null ? void 0 : _this$assembly.update();
         };
-        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);
         });
       },
     });
@@ -251,6 +252,7 @@ export default class Transloadit extends BasePlugin {
           const assembly = (_this$assembly2 = this.assembly) != null
             ? _this$assembly2
             : await _classPrivateFieldLooseBase(this, _createAssembly)[_createAssembly](fileIDs, assemblyOptions);
+          if (assembly == null) throw new Error("All files were canceled after assembly was created");
           if (this.opts.importFromUploadURLs) {
             await _classPrivateFieldLooseBase(this, _reserveFiles)[_reserveFiles](assembly, fileIDs);
           }
@@ -274,46 +276,41 @@ export default class Transloadit extends BasePlugin {
     });
     Object.defineProperty(this, _afterUpload, {
       writable: true,
-      value: (fileIDs, uploadID) => {
-        var _this$assembly3;
-        const files = fileIDs.map(fileID => this.uppy.getFile(fileID));
-        const filteredFileIDs = files.filter(file => !file.error).map(file => file.id);
-        if (this.restored) {
-          return this.restored.then(() => {
-            return _classPrivateFieldLooseBase(this, _afterUpload)[_afterUpload](filteredFileIDs, uploadID);
-          });
-        }
-        const assemblyID = (_this$assembly3 = this.assembly) == null ? void 0 : _this$assembly3.status.assembly_id;
-        const closeSocketConnections = () => {
-          var _this$assembly4;
-          (_this$assembly4 = this.assembly) == null || _this$assembly4.close();
-        };
-        if (!_classPrivateFieldLooseBase(this, _shouldWaitAfterUpload)[_shouldWaitAfterUpload]()) {
-          var _this$assembly5;
-          closeSocketConnections();
-          const status = (_this$assembly5 = this.assembly) == null ? void 0 : _this$assembly5.status;
-          if (status != null) {
+      value: async (fileIDs, uploadID) => {
+        try {
+          var _this$assembly3, _this$assembly6;
+          await this.restored;
+          const files = fileIDs.map(fileID => this.uppy.getFile(fileID)).filter(file => !file.error);
+          const assemblyID = (_this$assembly3 = this.assembly) == null ? void 0 : _this$assembly3.status.assembly_id;
+          const closeSocketConnections = () => {
+            var _this$assembly4;
+            (_this$assembly4 = this.assembly) == null || _this$assembly4.close();
+          };
+          if (!_classPrivateFieldLooseBase(this, _shouldWaitAfterUpload)[_shouldWaitAfterUpload]()) {
+            var _this$assembly5;
+            closeSocketConnections();
+            const status = (_this$assembly5 = this.assembly) == null ? void 0 : _this$assembly5.status;
+            if (status != null) {
+              this.uppy.addResultData(uploadID, {
+                transloadit: [status],
+              });
+            }
+            return;
+          }
+          if (!assemblyID) {
             this.uppy.addResultData(uploadID, {
-              transloadit: [status],
+              transloadit: [],
             });
+            return;
           }
-          return Promise.resolve();
-        }
-        if (!assemblyID) {
-          this.uppy.addResultData(uploadID, {
-            transloadit: [],
-          });
-          return Promise.resolve();
-        }
-        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 _classPrivateFieldLooseBase(this, _watcher)[_watcher].promise.then(() => {
-          var _this$assembly6;
+          await _classPrivateFieldLooseBase(this, _watcher)[_watcher].promise;
           closeSocketConnections();
           const status = (_this$assembly6 = this.assembly) == null ? void 0 : _this$assembly6.status;
           if (status != null) {
@@ -321,7 +318,9 @@ export default class Transloadit extends BasePlugin {
               transloadit: [status],
             });
           }
-        });
+        } finally {
+          this.assembly = undefined;
+        }
       },
     });
     Object.defineProperty(this, _closeAssemblyIfExists, {
@@ -507,12 +506,13 @@ function _attachAssemblyMetadata2(file, status) {
   }
   return newFile;
 }
-function _createAssembly2(fileIDs, assemblyOptions) {
+async function _createAssembly2(fileIDs, assemblyOptions) {
   this.uppy.log("[Transloadit] Create Assembly");
-  return this.client.createAssembly({
-    ...assemblyOptions,
-    expectedFiles: fileIDs.length,
-  }).then(async newAssembly => {
+  try {
+    const newAssembly = await this.client.createAssembly({
+      ...assemblyOptions,
+      expectedFiles: fileIDs.length,
+    });
     const files = this.uppy.getFiles().filter(_ref => {
       let {
         id,
@@ -544,7 +544,7 @@ function _createAssembly2(fileIDs, assemblyOptions) {
     this.uppy.emit("transloadit:assembly-created", status, fileIDs);
     this.uppy.log(`[Transloadit] Created Assembly ${assemblyID}`);
     return assembly;
-  }).catch(err => {
+  } catch (err) {
     const wrapped = new ErrorWithCause(`${this.i18n("creatingAssemblyFailed")}: ${err.message}`, {
       cause: err,
     });
@@ -555,7 +555,7 @@ function _createAssembly2(fileIDs, assemblyOptions) {
       wrapped.assembly = err.assembly;
     }
     throw wrapped;
-  });
+  }
 }
 function _createAssemblyWatcher2(idOrArrayOfIds) {
   const ids = Array.isArray(idOrArrayOfIds) ? idOrArrayOfIds : [idOrArrayOfIds];
@@ -655,6 +655,7 @@ function _onAssemblyFinished2(assembly) {
 async function _cancelAssembly2(assembly) {
   await this.client.cancelAssembly(assembly);
   this.uppy.emit("transloadit:assembly-cancelled", assembly);
+  this.assembly = undefined;
 }
 function _connectAssembly2(assembly) {
   const {

Base automatically changed from null-assertion-fixes to main August 15, 2024 17:35
fixes #5397
also refactor from promise.then to async/await
and fix what seems like broken logic with recursive this.#afterUpload call
after an assembly has been created
also rewrite #createAssembly to async/await
fixes potential race condition
@aduh95 aduh95 force-pushed the fix-transloadit-allowMultipleUploadBatches branch from a457a64 to 3456c23 Compare August 15, 2024 17:39
Comment on lines -68 to +75
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those changes are purely aesthetic, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, because plugin.assembly is now set to undefined after an upload has finished, we need to get the assembly out first, or else the test was failing due to assembly being undefined on plugin

@aduh95 aduh95 merged commit 013f4ea into main Aug 15, 2024
17 checks passed
@aduh95 aduh95 deleted the fix-transloadit-allowMultipleUploadBatches branch August 15, 2024 18:10
github-actions bot added a commit that referenced this pull request Aug 16, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   4.0.3 | @uppy/provider-views   |   4.0.1 |
| @uppy/companion        |   5.0.5 | @uppy/status-bar       |   4.0.2 |
| @uppy/companion-client |   4.0.1 | @uppy/transloadit      |   4.0.2 |
| @uppy/core             |   4.1.1 | @uppy/tus              |   4.0.1 |
| @uppy/dashboard        |   4.0.3 | @uppy/utils            |   6.0.2 |
| @uppy/drag-drop        |   4.0.2 | @uppy/vue              |   2.0.1 |
| @uppy/file-input       |   4.0.1 | uppy                   |   4.1.1 |
| @uppy/image-editor     |   3.0.1 |                        |         |

- @uppy/transloadit: fix issue with `allowMultipleUploadBatches` (Mikael Finstad / #5400)
- meta: Bump elliptic from 6.5.5 to 6.5.7 (dependabot[bot] / #5410)
- meta: add back patch for `p-queue` (Antoine du Hamel / #5409)
- @uppy/transloadit: fix many lurking `TypeError` (Mikael Finstad / #5399)
- docs: improve `corsOrigins` documentation (Mikael Finstad / #5390)
- docs: add `ViewEncapsulation` to Angular example (Aaron Russell / #5395)
- @uppy/companion: fix code for custom providers (Mikael Finstad / #5398)
- docs: add note about throwing in `cancelAll` and `destroy()` (Mikael Finstad / #5408)
- meta: Bump docker/login-action from 3.2.0 to 3.3.0 (dependabot[bot] / #5372)
- meta: Bump docker/setup-qemu-action from 3.1.0 to 3.2.0 (dependabot[bot] / #5370)
- docs: make hosted Companion more clear (Merlijn Vos / #5394)
- meta: Bump docker/build-push-action from 6.4.1 to 6.6.1 (dependabot[bot] / #5403)
- meta: bump p-queue to latest, remove patch (Mikael Finstad / #5391)
- meta: enforce `.ts` extension for relative import types (Antoine du Hamel / #5393)
- @uppy/tus: Fix onShouldRetry type signature (Trent Nadeau / #5387)
- @uppy/dashboard,@uppy/drag-drop,@uppy/file-input: Transform the `accept` prop into a string everywhere (Evgenia Karunus / #5380)
- docs: fix getTemporarySecurityCredentials in aws-s3 (Merlijn Vos / #5363)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading additional batches with Transloadit is broken allowMultipleUploadBatches: true
2 participants