DEV: Upgrade Uppy to v4 (#29397)

Key changes include:

- `@uppy/aws-s3-multipart` is now part of `@uppy/aws-s3`, and controlled with a boolean

- Some minor changes/renames to Uppy APIs

- Uppy has removed batch signing from their S3 multipart implementation. This commit implements a batching system outside of Uppy to avoid needing one-signing-request-per-part

- Reduces concurrent part uploads to 6, because S3 uses HTTP/1.1 and browsers limit concurrent connections to 6-per-host.

- Upstream drop-target implementation has changed slightly, so we now need `pointer-events: none` on the hover element

app/assets/javascripts/discourse-common/package.json

@@ -16,12 +16,6 @@
   "dependencies": {
     "@babel/core": "^7.26.0",
     "@ember/string": "^4.0.0",
-    "@uppy/aws-s3": "3.0.6",
-    "@uppy/aws-s3-multipart": "3.1.3",
-    "@uppy/core": "3.0.4",
-    "@uppy/drop-target": "2.0.1",
-    "@uppy/utils": "5.4.3",
-    "@uppy/xhr-upload": "3.1.1",
     "discourse-i18n": "workspace:1.0.0",
     "ember-auto-import": "^2.8.1",
     "ember-cli-babel": "^8.2.0",

app/assets/javascripts/discourse/app/lib/uploads.js

@@ -342,10 +342,18 @@ export function displayErrorForUpload(data, siteSettings, fileName) {
     if (didError) {
       return;
     }
-  } else if (data.body && data.status) {
+  } else if (data.responseText && data.status) {
+    let parsedBody = data.responseText;
+    if (typeof parsedBody === "string") {
+      try {
+        parsedBody = JSON.parse(parsedBody);
+      } catch (e) {
+        // ignore
+      }
+    }
     const didError = displayErrorByResponseStatus(
       data.status,
-      data.body,
+      parsedBody,
       fileName,
       siteSettings
     );

app/assets/javascripts/discourse/app/lib/uppy/composer-upload.js

@@ -117,7 +117,7 @@ export default class UppyComposerUpload {
     this.#reset();
 
     if (this.uppyWrapper.uppyInstance) {
-      this.uppyWrapper.uppyInstance.close();
+      this.uppyWrapper.uppyInstance.destroy();
       this.uppyWrapper.uppyInstance = null;
     }
 
@@ -311,13 +311,9 @@ export default class UppyComposerUpload {
       });
     });
 
-    this.uppyWrapper.uppyInstance.on("upload", (data) => {
+    this.uppyWrapper.uppyInstance.on("upload", (uploadId, files) => {
       run(() => {
-        this.uppyWrapper.addNeedProcessing(data.fileIDs.length);
+        this.uppyWrapper.addNeedProcessing(files.length);
-
-        const files = data.fileIDs.map((fileId) =>
-          this.uppyWrapper.uppyInstance.getFile(fileId)
-        );
 
         this.composer.setProperties({
           isProcessingUpload: true,
@@ -605,6 +601,7 @@ export default class UppyComposerUpload {
   #useXHRUploads() {
     this.uppyWrapper.uppyInstance.use(XHRUpload, {
       endpoint: getURL(`/uploads.json?client_id=${this.messageBus.clientId}`),
+      shouldRetry: () => false,
       headers: () => ({
         "X-CSRF-Token": this.session.csrfToken,
       }),
@@ -627,7 +624,7 @@ export default class UppyComposerUpload {
   }
 
   #resetUpload(file, opts) {
-    if (opts.removePlaceholder) {
+    if (opts.removePlaceholder && this.#placeholders[file.id]) {
       this.appEvents.trigger(
         `${this.composerEventPrefix}:replace-text`,
         this.#placeholders[file.id].uploadPlaceholder,

app/assets/javascripts/discourse/app/lib/uppy/s3-multipart.js

@@ -1,12 +1,15 @@
 import { setOwner } from "@ember/owner";
+import { debounce } from "@ember/runloop";
 import { service } from "@ember/service";
-import AwsS3Multipart from "@uppy/aws-s3-multipart";
+import AwsS3 from "@uppy/aws-s3";
 import { Promise } from "rsvp";
 import { ajax } from "discourse/lib/ajax";
 
 const RETRY_DELAYS = [0, 1000, 3000, 5000];
 const MB = 1024 * 1024;
 
+const s3MultipartMeta = new WeakMap(); // file -> { attempts: { partNumber -> attempts }, signingErrorRaised: boolean, batchSigner: BatchSigner }
+
 export default class UppyS3Multipart {
   @service siteSettings;
 
@@ -20,15 +23,17 @@ export default class UppyS3Multipart {
   apply(uppyInstance) {
     this.uppyInstance = uppyInstance;
 
-    this.uppyInstance.use(AwsS3Multipart, {
+    this.uppyInstance.use(AwsS3, {
-      // controls how many simultaneous _chunks_ are uploaded, not files,
+      // TODO: using multipart even for tiny files is not ideal. Now that uppy
-      // which in turn controls the minimum number of chunks presigned
+      // made multipart a simple boolean, rather than a separate plugin, we can
-      // in each batch (limit / 2)
+      // consider combining our two S3 implementations and choose the strategy
-      //
+      // based on file size.
-      // the default, and minimum, chunk size is 5mb. we can control the
+      shouldUseMultipart: true,
-      // chunk size via getChunkSize(file), so we may want to increase
+
-      // the chunk size for larger files
+      // Number of concurrent part uploads. AWS uses http/1.1,
-      limit: 10,
+      // which browsers limit to 6 concurrent connections per host.
+      limit: 6,
+
       retryDelays: RETRY_DELAYS,
 
       // When we get to really big files, it's better to not have thousands
@@ -46,9 +51,9 @@ export default class UppyS3Multipart {
       },
 
       createMultipartUpload: this.#createMultipartUpload.bind(this),
-      prepareUploadParts: this.#prepareUploadParts.bind(this),
       completeMultipartUpload: this.#completeMultipartUpload.bind(this),
       abortMultipartUpload: this.#abortMultipartUpload.bind(this),
+      signPart: this.#signPart.bind(this),
 
       // we will need a listParts function at some point when we want to
       // resume multipart uploads; this is used by uppy to figure out
@@ -89,54 +94,56 @@ export default class UppyS3Multipart {
     });
   }
 
-  #prepareUploadParts(file, partData) {
+  #getFileMeta(file) {
-    if (file.preparePartsRetryAttempts === undefined) {
+    if (s3MultipartMeta.has(file)) {
-      file.preparePartsRetryAttempts = 0;
+      return s3MultipartMeta.get(file);
     }
-    return ajax(`${this.uploadRootPath}/batch-presign-multipart-parts.json`, {
-      type: "POST",
-      data: {
-        part_numbers: partData.parts.map((part) => part.number),
-        unique_identifier: file.meta.unique_identifier,
-      },
-    })
-      .then((data) => {
-        if (file.preparePartsRetryAttempts) {
-          delete file.preparePartsRetryAttempts;
-          this.uppyWrapper.debug.log(
-            `[uppy] Retrying batch fetch for ${file.id} was successful, continuing.`
-          );
-        }
-        return { presignedUrls: data.presigned_urls };
-      })
-      .catch((err) => {
-        const status = err.jqXHR.status;
 
-        // it is kind of ugly to have to track the retry attempts for
+    const fileMeta = {
-        // the file based on the retry delays, but uppy's `retryable`
+      attempts: {},
-        // function expects the rejected Promise data to be structured
+      signingErrorRaised: false,
-        // _just so_, and provides no interface for us to tell how many
+      batchSigner: new BatchSigner({
-        // times the upload has been retried (which it tracks internally)
+        file,
-        //
+        uploadRootPath: this.uploadRootPath,
-        // if we exceed the attempts then there is no way that uppy will
+      }),
-        // retry the upload once again, so in that case the alert can
+    };
-        // be safely shown to the user that their upload has failed.
+
-        if (file.preparePartsRetryAttempts < RETRY_DELAYS.length) {
+    s3MultipartMeta.set(file, fileMeta);
-          file.preparePartsRetryAttempts += 1;
+    return fileMeta;
-          const attemptsLeft =
+  }
-            RETRY_DELAYS.length - file.preparePartsRetryAttempts + 1;
+
-          this.uppyWrapper.debug.log(
+  async #signPart(file, partData) {
-            `[uppy] Fetching a batch of upload part URLs for ${file.id} failed with status ${status}, retrying ${attemptsLeft} more times...`
+    const fileMeta = this.#getFileMeta(file);
-          );
+
-          return Promise.reject({ source: { status } });
+    fileMeta.attempts[partData.partNumber] ??= 0;
-        } else {
+    const thisPartAttempts = (fileMeta.attempts[partData.partNumber] += 1);
-          this.uppyWrapper.debug.log(
+
-            `[uppy] Fetching a batch of upload part URLs for ${file.id} failed too many times, throwing error.`
+    this.uppyWrapper.debug.log(
-          );
+      `[uppy] requesting signature for part ${partData.partNumber} (attempt ${thisPartAttempts})`
-          // uppy is inconsistent, an error here does not fire the upload-error event
+    );
-          this.handleUploadError(file, err);
+
-        }
+    try {
-      });
+      const url = await fileMeta.batchSigner.signedUrlFor(partData);
+      this.uppyWrapper.debug.log(
+        `[uppy] signature for part ${partData.partNumber} obtained, continuing.`
+      );
+      return { url };
+    } catch (err) {
+      // Uppy doesn't properly bubble errors from failed #signPart, so we call
+      // the error handler ourselves after the last failed attempt
+      if (
+        !fileMeta.signingErrorRaised &&
+        thisPartAttempts >= RETRY_DELAYS.length
+      ) {
+        this.uppyWrapper.debug.log(
+          `[uppy] Fetching a signed part URL for ${file.id} failed too many times, raising error.`
+        );
+        // uppy is inconsistent, an error here does not fire the upload-error event
+        this.handleUploadError(file, err);
+        fileMeta.signingErrorRaised = true;
+      }
+      throw err;
+    }
   }
 
   #completeMultipartUpload(file, data) {
@@ -193,3 +200,78 @@ export default class UppyS3Multipart {
     });
   }
 }
+
+const BATCH_SIGNER_INITIAL_DEBOUNCE = 50;
+const BATCH_SIGNER_REGULAR_DEBOUNCE = 500;
+
+/**
+ * This class is responsible for batching requests to the server to sign
+ * parts of a multipart upload. It is used to avoid making a request for
+ * every single part, which would likely hit our rate limits.
+ */
+class BatchSigner {
+  pendingRequests = [];
+  #madeFirstRequest = false;
+
+  constructor({ file, uploadRootPath }) {
+    this.file = file;
+    this.uploadRootPath = uploadRootPath;
+  }
+
+  signedUrlFor(partData) {
+    const promise = new Promise((resolve, reject) => {
+      this.pendingRequests.push({
+        partData,
+        resolve,
+        reject,
+      });
+    });
+
+    this.#scheduleSigning();
+    return promise;
+  }
+
+  #scheduleSigning() {
+    debounce(
+      this,
+      this.#signParts,
+      this.#madeFirstRequest
+        ? BATCH_SIGNER_REGULAR_DEBOUNCE
+        : BATCH_SIGNER_INITIAL_DEBOUNCE
+    );
+  }
+
+  async #signParts() {
+    if (this.pendingRequests.length === 0) {
+      return;
+    }
+
+    this.#madeFirstRequest = true;
+
+    const requests = this.pendingRequests;
+    this.pendingRequests = [];
+
+    try {
+      const result = await ajax(
+        `${this.uploadRootPath}/batch-presign-multipart-parts.json`,
+        {
+          type: "POST",
+          data: {
+            part_numbers: requests.map(
+              (request) => request.partData.partNumber
+            ),
+            unique_identifier: this.file.meta.unique_identifier,
+          },
+        }
+      );
+      requests.forEach(({ partData, resolve }) => {
+        resolve(result.presigned_urls[partData.partNumber.toString()]);
+      });
+    } catch (err) {
+      // eslint-disable-next-line no-console
+      console.error("[uppy] failed to get part signatures", err);
+      requests.forEach(({ reject }) => reject(err));
+      return;
+    }
+  }
+}

app/assets/javascripts/discourse/app/lib/uppy/upload-debugging.js

@@ -70,10 +70,8 @@ export default class UppyUploadDebugging {
       return;
     }
 
-    uppy.on("upload", (data) => {
+    uppy.on("upload", (uploadID, files) => {
-      data.fileIDs.forEach((fileId) =>
+      files.forEach(({ id }) => this.#performanceMark(`upload-${id}-start`));
-        this.#performanceMark(`upload-${fileId}-start`)
-      );
     });
 
     uppy.on("create-multipart", (fileId) => {

app/assets/javascripts/discourse/app/lib/uppy/uppy-upload.js

@@ -210,11 +210,8 @@ export default class UppyUpload {
       this.uploadProgress = progress;
     });
 
-    this.uppyWrapper.uppyInstance.on("upload", (data) => {
+    this.uppyWrapper.uppyInstance.on("upload", (uploadId, files) => {
-      this.uppyWrapper.addNeedProcessing(data.fileIDs.length);
+      this.uppyWrapper.addNeedProcessing(files.length);
-      const files = data.fileIDs.map((fileId) =>
-        this.uppyWrapper.uppyInstance.getFile(fileId)
-      );
       this.processing = true;
       this.cancellable = false;
       files.forEach((file) => {
@@ -287,6 +284,9 @@ export default class UppyUpload {
     this.uppyWrapper.uppyInstance.on(
       "upload-error",
       (file, error, response) => {
+        if (response.aborted) {
+          return; // User cancelled the upload
+        }
         this.#removeInProgressUpload(file.id);
         displayErrorForUpload(response || error, this.siteSettings, file.name);
         this.#reset();
@@ -402,6 +402,7 @@ export default class UppyUpload {
   #useXHRUploads() {
     this.uppyWrapper.uppyInstance.use(XHRUpload, {
       endpoint: this.#xhrUploadUrl(),
+      shouldRetry: () => false,
       headers: () => ({
         "X-CSRF-Token": this.session.csrfToken,
       }),
@@ -420,6 +421,7 @@ export default class UppyUpload {
   #useS3Uploads() {
     this.#usingS3Uploads = true;
     this.uppyWrapper.uppyInstance.use(AwsS3, {
+      shouldUseMultipart: false,
       getUploadParameters: (file) => {
         const data = {
           file_name: file.name,

app/assets/javascripts/discourse/package.json

@@ -60,12 +60,11 @@
     "@types/jquery": "^3.5.32",
     "@types/qunit": "^2.19.11",
     "@types/rsvp": "^4.0.9",
-    "@uppy/aws-s3": "3.0.6",
+    "@uppy/aws-s3": "^4.1.0",
-    "@uppy/aws-s3-multipart": "3.1.3",
+    "@uppy/core": "^4.2.2",
-    "@uppy/core": "3.0.4",
+    "@uppy/drop-target": "3.0.1",
-    "@uppy/drop-target": "2.0.1",
+    "@uppy/utils": "^6.0.3",
-    "@uppy/utils": "5.4.3",
+    "@uppy/xhr-upload": "^4.2.1",
-    "@uppy/xhr-upload": "3.1.1",
     "a11y-dialog": "8.1.1",
     "admin": "workspace:1.0.0",
     "autosize": "^6.0.1",

app/assets/javascripts/discourse/tests/acceptance/composer-uploads-uppy-test.js

@@ -498,14 +498,6 @@ acceptance("Uppy Composer Attachment - Upload Error", function (needs) {
   });
 
   test("should show an error message for the failed upload", async function (assert) {
-    // Don't log the upload error
-    const stub = sinon
-      .stub(console, "error")
-      .withArgs(
-        sinon.match(/\[Uppy\]/),
-        sinon.match(/Failed to upload avatar\.png/)
-      );
-
     await visit("/");
     await click("#create-topic");
     await fillIn(".d-editor-input", "The image:\n");
@@ -513,7 +505,6 @@ acceptance("Uppy Composer Attachment - Upload Error", function (needs) {
     const done = assert.async();
 
     appEvents.on("composer:upload-error", async () => {
-      sinon.assert.calledOnce(stub);
       await settled();
       assert.strictEqual(
         query(".dialog-body").textContent.trim(),

app/assets/javascripts/discourse/tests/unit/lib/uploads-test.js

@@ -444,7 +444,7 @@ module("Unit | Utility | uploads", function (hooks) {
     displayErrorForUpload(
       {
         status: 422,
-        body: { message: "upload failed" },
+        responseText: JSON.stringify({ message: "upload failed" }),
       },
       "test.png",
       { max_attachment_size_kb: 1024, max_image_size_kb: 1024 }

app/assets/stylesheets/common/admin/admin_emojis.scss

@@ -11,6 +11,7 @@
   }
   .uppy-is-drag-over {
     box-shadow: 0 0px 52px 0 #ffffff, 0px 7px 33px 0 var(--tertiary-low);
+    pointer-events: none;
   }
   #custom_emoji.highlighted {
     background: var(--tertiary-very-low);

plugins/chat/assets/stylesheets/common/base-common.scss

@@ -26,6 +26,7 @@ html.ios-device.keyboard-visible body #main-outlet .full-page-chat {
   left: 0;
   background-color: rgba(0, 0, 0, 0.75);
   z-index: z("header");
+  pointer-events: none;
   &-content {
     width: max-content;
     display: flex;

plugins/chat/assets/stylesheets/common/chat-upload-drop-zone.scss

@@ -10,6 +10,7 @@
   justify-content: center;
   display: flex;
   background: rgba(var(--always-black-rgb), 0.85);
+  pointer-events: none;
 
   .uppy-is-drag-over & {
     visibility: visible;

plugins/chat/test/javascripts/components/chat-composer-uploads-test.js

@@ -120,9 +120,8 @@ module("Discourse Chat | Component | chat-composer-uploads", function (hooks) {
     this.appEvents.on(
       `upload-mixin:chat-composer-uploader:upload-cancelled`,
       (fileId) => {
-        assert.strictEqual(
+        assert.true(
-          fileId.includes("uppy-avatar/"),
+          fileId.includes("chat-composer-uploader-avatar/"),
-          true,
           "upload was cancelled"
         );
         done();