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: remove reason #5159

Merged
merged 5 commits into from
May 23, 2024
Merged

@uppy/core: remove reason #5159

merged 5 commits into from
May 23, 2024

Conversation

aduh95
Copy link
Member

@aduh95 aduh95 commented May 14, 2024

We should also update the React example who is still using it.

@aduh95 aduh95 requested a review from Murderlon May 14, 2024 08:25
Copy link
Contributor

github-actions bot commented May 14, 2024

Diff output files
diff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index 1b136e1..856e4de 100644
--- a/packages/@uppy/aws-s3/lib/index.js
+++ b/packages/@uppy/aws-s3/lib/index.js
@@ -613,13 +613,11 @@ function _uploadLocalFile2(file) {
       });
       resolve(`upload ${removed} was removed`);
     });
-    eventManager.onCancelAll(file.id, options => {
-      if ((options == null ? void 0 : options.reason) === "user") {
-        upload.abort();
-        this.resetUploaderReferences(file.id, {
-          abort: true,
-        });
-      }
+    eventManager.onCancelAll(file.id, () => {
+      upload.abort();
+      this.resetUploaderReferences(file.id, {
+        abort: true,
+      });
       resolve(`upload ${file.id} was canceled`);
     });
     eventManager.onFilePause(file.id, isPaused => {
diff --git a/packages/@uppy/companion-client/lib/RequestClient.js b/packages/@uppy/companion-client/lib/RequestClient.js
index a7ef904..6d54db2 100644
--- a/packages/@uppy/companion-client/lib/RequestClient.js
+++ b/packages/@uppy/companion-client/lib/RequestClient.js
@@ -453,14 +453,9 @@ async function _awaitRemoteFileUpload2(_ref4) {
         this.uppy.log(`upload ${file.id} was removed`, "info");
         resolve();
       };
-      const onCancelAll = _ref5 => {
+      const onCancelAll = () => {
         var _socketAbortControlle5;
-        let {
-          reason,
-        } = _ref5;
-        if (reason === "user") {
-          socketSend("cancel");
-        }
+        socketSend("cancel");
         (_socketAbortControlle5 = socketAbortController) == null || _socketAbortControlle5.abort == null
           || _socketAbortControlle5.abort();
         this.uppy.log(`upload ${file.id} was canceled`, "info");
diff --git a/packages/@uppy/core/lib/Uppy.js b/packages/@uppy/core/lib/Uppy.js
index 7feaf32..aaf0e13 100644
--- a/packages/@uppy/core/lib/Uppy.js
+++ b/packages/@uppy/core/lib/Uppy.js
@@ -566,7 +566,7 @@ export class Uppy {
       _classPrivateFieldLooseBase(this, _startIfAutoProceed)[_startIfAutoProceed]();
     }
   }
-  removeFiles(fileIDs, reason) {
+  removeFiles(fileIDs) {
     const {
       files,
       currentUploads,
@@ -617,7 +617,7 @@ export class Uppy {
     this.calculateTotalProgress();
     const removedFileIDs = Object.keys(removedFiles);
     removedFileIDs.forEach(fileID => {
-      this.emit("file-removed", removedFiles[fileID], reason);
+      this.emit("file-removed", removedFiles[fileID]);
     });
     if (removedFileIDs.length > 5) {
       this.log(`Removed ${removedFileIDs.length} files`);
@@ -625,8 +625,8 @@ export class Uppy {
       this.log(`Removed files: ${removedFileIDs.join(", ")}`);
     }
   }
-  removeFile(fileID, reason) {
-    this.removeFiles([fileID], reason);
+  removeFile(fileID) {
+    this.removeFiles([fileID]);
   }
   pauseResume(fileID) {
     if (!this.getState().capabilities.resumableUploads || this.getFile(fileID).progress.uploadComplete) {
@@ -710,23 +710,16 @@ export class Uppy {
     });
     return _classPrivateFieldLooseBase(this, _runUpload)[_runUpload](uploadID);
   }
-  cancelAll(_temp) {
-    let {
-      reason = "user",
-    } = _temp === void 0 ? {} : _temp;
-    this.emit("cancel-all", {
-      reason,
-    });
-    if (reason === "user") {
-      const {
-        files,
-      } = this.getState();
-      const fileIDs = Object.keys(files);
-      if (fileIDs.length) {
-        this.removeFiles(fileIDs, "cancel-all");
-      }
-      this.setState(defaultUploadState);
+  cancelAll() {
+    this.emit("cancel-all");
+    const {
+      files,
+    } = this.getState();
+    const fileIDs = Object.keys(files);
+    if (fileIDs.length) {
+      this.removeFiles(fileIDs);
     }
+    this.setState(defaultUploadState);
   }
   retryUpload(fileID) {
     this.setFileState(fileID, {
@@ -880,14 +873,9 @@ export class Uppy {
     };
     this.setState(updatedState);
   }
-  destroy(_temp2) {
-    let {
-      reason,
-    } = _temp2 === void 0 ? {} : _temp2;
+  destroy() {
     this.log(`Closing Uppy instance ${this.opts.id}: removing all files and uninstalling plugins`);
-    this.cancelAll({
-      reason,
-    });
+    this.cancelAll();
     _classPrivateFieldLooseBase(this, _storeUnsubscribe)[_storeUnsubscribe]();
     this.iteratePlugins(plugin => {
       this.removePlugin(plugin);
diff --git a/packages/@uppy/transloadit/lib/index.js b/packages/@uppy/transloadit/lib/index.js
index 863f8d6..1f4cdd4 100644
--- a/packages/@uppy/transloadit/lib/index.js
+++ b/packages/@uppy/transloadit/lib/index.js
@@ -65,12 +65,10 @@ var _onError = _classPrivateFieldLooseKey("onError");
 var _onTusError = _classPrivateFieldLooseKey("onTusError");
 export default class Transloadit extends BasePlugin {
   constructor(uppy, opts) {
-    var _this;
     super(uppy, {
       ...defaultOptions,
       ...opts,
     });
-    _this = this;
     Object.defineProperty(this, _connectAssembly, {
       value: _connectAssembly2,
     });
@@ -131,22 +129,18 @@ export default class Transloadit extends BasePlugin {
     });
     Object.defineProperty(this, _onCancelAll, {
       writable: true,
-      value: async function(_temp) {
-        let {
-          reason,
-        } = _temp === void 0 ? {} : _temp;
+      value: async () => {
         try {
-          if (reason !== "user") return;
           const {
             uploadsAssemblies,
-          } = _this.getPluginState();
+          } = this.getPluginState();
           const assemblyIDs = Object.values(uploadsAssemblies).flat(1);
-          const assemblies = assemblyIDs.map(assemblyID => _this.getAssembly(assemblyID));
+          const assemblies = assemblyIDs.map(assemblyID => this.getAssembly(assemblyID));
           await Promise.all(
-            assemblies.map(assembly => _classPrivateFieldLooseBase(_this, _cancelAssembly)[_cancelAssembly](assembly)),
+            assemblies.map(assembly => _classPrivateFieldLooseBase(this, _cancelAssembly)[_cancelAssembly](assembly)),
           );
         } catch (err) {
-          _this.uppy.log(err);
+          this.uppy.log(err);
         }
       },
     });
@@ -614,16 +608,13 @@ function _createAssembly2(fileIDs, uploadID, assemblyOptions) {
         ...updatedFiles,
       },
     });
-    const fileRemovedHandler = (fileRemoved, reason) => {
+    const fileRemovedHandler = fileRemoved => {
       var _assembly$status;
       if (((_assembly$status = assembly.status) == null ? void 0 : _assembly$status.ok) === "ASSEMBLY_COMPLETED") {
         this.uppy.off("file-removed", fileRemovedHandler);
         return;
       }
-      if (reason === "cancel-all") {
-        assembly.close();
-        this.uppy.off("file-removed", fileRemovedHandler);
-      } else if (fileRemoved.id in updatedFiles) {
+      if (fileRemoved.id in updatedFiles) {
         delete updatedFiles[fileRemoved.id];
         const nbOfRemainingFiles = Object.keys(updatedFiles).length;
         this.client.updateNumberOfFilesInAssembly(newAssembly, nbOfRemainingFiles).catch(() => {});
diff --git a/packages/@uppy/tus/lib/index.js b/packages/@uppy/tus/lib/index.js
index 22083e0..1d645f2 100644
--- a/packages/@uppy/tus/lib/index.js
+++ b/packages/@uppy/tus/lib/index.js
@@ -178,7 +178,6 @@ export default class Tus extends BasePlugin {
   }
 }
 function _uploadLocalFile2(file) {
-  var _this = this;
   this.resetUploaderReferences(file.id);
   return new Promise((resolve, reject) => {
     let queuedRequest;
@@ -374,16 +373,11 @@ function _uploadLocalFile2(file) {
       queuedRequest.abort();
       upload.abort();
     });
-    eventManager.onCancelAll(file.id, function(_temp) {
-      let {
-        reason,
-      } = _temp === void 0 ? {} : _temp;
-      if (reason === "user") {
-        queuedRequest.abort();
-        _this.resetUploaderReferences(file.id, {
-          abort: !!upload.url,
-        });
-      }
+    eventManager.onCancelAll(file.id, () => {
+      queuedRequest.abort();
+      this.resetUploaderReferences(file.id, {
+        abort: !!upload.url,
+      });
       resolve(`upload ${file.id} was canceled`);
     });
     eventManager.onResumeAll(file.id, () => {
diff --git a/packages/@uppy/xhr-upload/lib/index.js b/packages/@uppy/xhr-upload/lib/index.js
index 8211d47..11050ce 100644
--- a/packages/@uppy/xhr-upload/lib/index.js
+++ b/packages/@uppy/xhr-upload/lib/index.js
@@ -290,13 +290,8 @@ async function _uploadLocalFile2(file) {
     });
   });
   events.onFileRemove(file.id, () => controller.abort());
-  events.onCancelAll(file.id, _ref => {
-    let {
-      reason,
-    } = _ref;
-    if (reason === "user") {
-      controller.abort();
-    }
+  events.onCancelAll(file.id, () => {
+    controller.abort();
   });
   try {
     await uppyFetch().abortOn(controller.signal);

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

CI is failing and there is also custom logic to be removed in companion-client, tus, xhr, and transloadit

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label May 15, 2024
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels May 15, 2024
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

What about this?

I'm currently investigating this code because this is likely a problem in production for a customer, all that code should be moved to cancel-all. But I suppose in this PR you can remove the if statement.

if (reason === 'cancel-all') {
assembly.close()
this.uppy.off('file-removed', fileRemovedHandler)
} else if (fileRemoved.id in updatedFiles) {
delete updatedFiles[fileRemoved.id]
const nbOfRemainingFiles = Object.keys(updatedFiles).length
if (nbOfRemainingFiles === 0) {
assembly.close()
this.#cancelAssembly(newAssembly).catch(() => {
/* ignore potential errors */
})
this.uppy.off('file-removed', fileRemovedHandler)
} else {
this.client
.updateNumberOfFilesInAssembly(newAssembly, nbOfRemainingFiles)
.catch(() => {
/* ignore potential errors */
})
}
}
}

@aduh95
Copy link
Member Author

aduh95 commented May 22, 2024

I'm not so sure about 087ad5b, I recommend careful review

// If the assembly has successfully completed, we do not need these checks.
// Otherwise we may cancel an assembly after it already succeeded
if (assembly.status?.ok === 'ASSEMBLY_COMPLETED') {
this.uppy.off('file-removed', fileRemovedHandler)
return
}
if (reason === 'cancel-all') {
assembly.close()
Copy link
Member

Choose a reason for hiding this comment

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

If we remove this line here, then we need to listen for the cancel-all event and cancel all assemblies. Which is the one and only only use case when an assembly should be cancelled, instead of continuing to run in the background.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is on the 4.x branch, we can also leave it removed for now and fix this in #5158, where assembly handling is already changing and it would have to be done different there after the refactor.

@aduh95 aduh95 merged commit f72138f into transloadit:4.x May 23, 2024
17 checks passed
Murderlon added a commit that referenced this pull request May 28, 2024
* 4.x: (38 commits)
  docs: assume tree-shaking bundler is the most common case (#5160)
  @uppy/core: remove `reason` (#5159)
  Release: uppy@4.0.0-beta.9 (#5194)
  Release: uppy@3.25.5 (#5193)
  @uppy/companion: remove `chalk` from dependencies (#5178)
  @uppy/transloadit: do not cancel assembly when removing all files (#5191)
  @uppy/xhr-upload: fix regression for lowercase HTTP methods (#5179)
  meta: improve changelog generator (#5190)
  Release: uppy@4.0.0-beta.8 (#5189)
  examples: add SvelteKit example (#5181)
  @uppy/companion: fix missing `await`
  Release: uppy@3.25.4 (#5188)
  @uppy/svelte: do not attempt removing plugin before it's created (#5186)
  Update facebook.mdx
  fixup! @uppy/tus: fix no headers passed to companion if argument is a function (#5182)
  @uppy/core: resolve some (breaking) TODOs (#4824)
  @uppy/tus: fix no headers passed to companion if argument is a function (#5182)
  @uppy/companion: fix google drive gsuite export large size (#5144)
  @uppy/companion: encode `uploadId` (#5168)
  @uppy/companion: bump `express-session` (#5177)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants