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

Allow Transloadit-hosted Companion with other uploaders #5558

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Dec 19, 2024

Closes #5515

We want to allow people to only use Transloadit for Companion, even if they use another uploader. This makes using Transloadit less all-or-nothing.

Unfortunately there is no pretty way of doing this. Instead of separating Companion logic from uploaders, every uploader implements remote uploads themselves. It works like this:

  1. Install a remote plugin (such as @uppy/google-drive)
  2. The remote plugin creates a companion request client (@uppy/companion-client) and @uppy/provider-views
  3. Inside @uppy/provider-views we call uppy.registerRequestClient() to put the request client on core so it can be shared.
    • We need to store request clients by a unique ID, so we can share RequestClient instances across files
      this allows us to do rate limiting and synchronous operations like refreshing provider tokens
      example: refreshing tokens: if each file has their own requestclient,
      we don't have any way to synchronize all requests in order to block all requests, refresh the token, and unblock all requests and allow them to run with a the new access token.
  4. When the upload starts, uploader plugins (such as @uppy/aws-s3) filters the remote files and gets the instructions to tell Companion what uploader to use:

#getCompanionClientArgs(file: UppyFile<M, B>) {
return {
...file.remote?.body,
protocol: 's3-multipart',
size: file.data.size,
metadata: file.meta,
}
}
#upload = async (fileIDs: string[]) => {
if (fileIDs.length === 0) return undefined
const files = this.uppy.getFilesByIds(fileIDs)
const filesFiltered = filterNonFailedFiles(files)
const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
this.uppy.emit('upload-start', filesToEmit)
const promises = filesFiltered.map((file) => {
if (file.isRemote) {
const getQueue = () => this.requests
this.#setResumableUploadsCapability(false)
const controller = new AbortController()
const removedHandler = (removedFile: UppyFile<M, B>) => {
if (removedFile.id === file.id) controller.abort()
}
this.uppy.on('file-removed', removedHandler)
const uploadPromise = this.uppy
.getRequestClientForFile<RequestClient<M, B>>(file)
.uploadRemoteFile(file, this.#getCompanionClientArgs(file), {
signal: controller.signal,
getQueue,
})
this.requests.wrapSyncFunction(
() => {
this.uppy.off('file-removed', removedHandler)
},
{ priority: -1 },
)()
return uploadPromise
}
return this.#uploadLocalFile(file)
})


With this PR you can do this:

uppy.use(AwsS3, { /* ... */ })
uppy.use(Transloadit, { onlyRemoteFiles: true /* ... */ })

This is the least ugly way I could come up with.

This is possible due to a new value on Uppy's state, remoteUploader, which is set by the Transloadit plugin when onlyRemoteFiles is true.

You can also (ab)use this to allow users to change the remote uploader in other cases: uppy.setState({ remoteUploader: 'multipart' }).

@Murderlon Murderlon requested a review from mifi December 19, 2024 14:20
@Murderlon Murderlon self-assigned this Dec 19, 2024
Copy link
Contributor

github-actions bot commented Dec 19, 2024

Diff output files
diff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index b2bf3c6..118e3f2 100644
--- a/packages/@uppy/aws-s3/lib/index.js
+++ b/packages/@uppy/aws-s3/lib/index.js
@@ -693,10 +693,19 @@ function _uploadLocalFile2(file) {
   });
 }
 function _getCompanionClientArgs2(file) {
-  var _file$remote;
+  var _file$remote, _tusOpts$endpoint;
+  const opts = {
+    ...this.opts,
+  };
+  const tusOpts = file.tus;
   return {
     ...((_file$remote = file.remote) == null ? void 0 : _file$remote.body),
-    protocol: "s3-multipart",
+    protocol: this.uppy.getState().remoteUploader || "s3-multipart",
+    endpoint: (_tusOpts$endpoint = tusOpts.endpoint) != null ? _tusOpts$endpoint : opts.endpoint,
+    headers: {
+      ...opts.headers,
+      ...tusOpts.headers,
+    },
     size: file.data.size,
     metadata: file.meta,
   };
diff --git a/packages/@uppy/transloadit/lib/index.js b/packages/@uppy/transloadit/lib/index.js
index 864333d..912ee5d 100644
--- a/packages/@uppy/transloadit/lib/index.js
+++ b/packages/@uppy/transloadit/lib/index.js
@@ -25,6 +25,7 @@ const defaultOptions = {
   waitForMetadata: false,
   alwaysRunAssembly: false,
   importFromUploadURLs: false,
+  onlyRemoteFiles: false,
   limit: 20,
   retryDelays: [7000, 10000, 15000, 20000],
   clientName: null,
@@ -248,25 +249,26 @@ export default class Transloadit extends BasePlugin {
           : this.opts.assemblyOptions;
         (_assemblyOptions$fiel = assemblyOptions.fields) != null ? _assemblyOptions$fiel : assemblyOptions.fields = {};
         validateParams(assemblyOptions.params);
+        const ids = this.opts.onlyRemoteFiles ? fileIDs.filter(id => this.uppy.getFile(id).isRemote) : fileIDs;
         try {
           var _this$assembly2;
           const assembly = (_this$assembly2 = this.assembly) != null
             ? _this$assembly2
-            : await _classPrivateFieldLooseBase(this, _createAssembly)[_createAssembly](fileIDs, assemblyOptions);
+            : await _classPrivateFieldLooseBase(this, _createAssembly)[_createAssembly](ids, 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);
+            await _classPrivateFieldLooseBase(this, _reserveFiles)[_reserveFiles](assembly, ids);
           }
-          fileIDs.forEach(fileID => {
+          ids.forEach(fileID => {
             const file = this.uppy.getFile(fileID);
             this.uppy.emit("preprocess-complete", file);
           });
           _classPrivateFieldLooseBase(this, _createAssemblyWatcher)[_createAssemblyWatcher](
             assembly.status.assembly_id,
           );
-          _classPrivateFieldLooseBase(this, _connectAssembly)[_connectAssembly](assembly, fileIDs);
+          _classPrivateFieldLooseBase(this, _connectAssembly)[_connectAssembly](assembly, ids);
         } catch (err) {
-          fileIDs.forEach(fileID => {
+          ids.forEach(fileID => {
             const file = this.uppy.getFile(fileID);
             this.uppy.emit("preprocess-complete", file);
             this.uppy.emit("upload-error", file, err);
@@ -361,6 +363,11 @@ export default class Transloadit extends BasePlugin {
     this.defaultLocale = locale;
     _classPrivateFieldLooseBase(this, _rateLimitedQueue)[_rateLimitedQueue] = new RateLimitedQueue(this.opts.limit);
     this.i18nInit();
+    if (this.opts.onlyRemoteFiles) {
+      this.uppy.setState({
+        remoteUploader: "tus",
+      });
+    }
     this.client = new Client({
       service: this.opts.service,
       client: _classPrivateFieldLooseBase(this, _getClientVersion)[_getClientVersion](),
@@ -380,7 +387,7 @@ export default class Transloadit extends BasePlugin {
         "upload-success",
         _classPrivateFieldLooseBase(this, _onFileUploadURLAvailable)[_onFileUploadURLAvailable],
       );
-    } else {
+    } else if (!this.opts.onlyRemoteFiles) {
       this.uppy.use(Tus, {
         storeFingerprintForResuming: false,
         allowedMetaFields: true,
diff --git a/packages/@uppy/xhr-upload/lib/index.js b/packages/@uppy/xhr-upload/lib/index.js
index 6245c94..f3585ac 100644
--- a/packages/@uppy/xhr-upload/lib/index.js
+++ b/packages/@uppy/xhr-upload/lib/index.js
@@ -361,19 +361,23 @@ async function _uploadBundle2(files) {
   }
 }
 function _getCompanionClientArgs2(file) {
-  var _file$remote;
+  var _file$remote, _tusOpts$endpoint;
   const opts = this.getOptions(file);
   const allowedMetaFields = getAllowedMetaFields(opts.allowedMetaFields, file.meta);
+  const tusOpts = file.tus;
   return {
     ...((_file$remote = file.remote) == null ? void 0 : _file$remote.body),
-    protocol: "multipart",
-    endpoint: opts.endpoint,
+    protocol: this.uppy.getState().remoteUploader || "multipart",
+    endpoint: (_tusOpts$endpoint = tusOpts.endpoint) != null ? _tusOpts$endpoint : opts.endpoint,
+    headers: {
+      ...opts.headers,
+      ...tusOpts.headers,
+    },
     size: file.data.size,
     fieldname: opts.fieldName,
     metadata: Object.fromEntries(allowedMetaFields.map(name => [name, file.meta[name]])),
     httpMethod: opts.method,
     useFormData: opts.formData,
-    headers: opts.headers,
   };
 }
 async function _uploadFiles2(files) {

Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

I think this is a very clever way to allow people to use Transloadit hosted Companion to upload local and remote files all the way to their destination. I'm afraid that this will further complicate our already very complex uploading/plugin logic, making it more error prone and even harder to understand, but I do see the great benefits so I think we have to do it even though it's hacky.

private/dev/Dashboard.js Outdated Show resolved Hide resolved
private/dev/Dashboard.js Show resolved Hide resolved
packages/@uppy/transloadit/src/index.ts Show resolved Hide resolved
packages/@uppy/aws-s3/src/index.ts Outdated Show resolved Hide resolved
packages/@uppy/xhr-upload/src/index.ts Outdated Show resolved Hide resolved
// Transloadit is only a pre and post processor.
// To let Transloadit hosted Companion download the file,
// we instruct any other upload plugin to use tus for remote uploads.
this.uppy.setState({ remoteUploader: 'tus' })
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like using state for this. Can't we instead make it a pure option on each uploader, instead of passing the transloadit plugin option through state like this? Yes it's more verbose but it's so much easier to grok when reading/understanding how the code works, compared to analysing state mutations. If the verbosity is a problem, that could be solved with good documation, or something like a helper method or wrapper plugin that will initialise the correct plugins with the correct options

Copy link
Member Author

Choose a reason for hiding this comment

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

How would that work? One uploader does not know if there is another uploader to set such a property. I could make a property on the uppy class instead of uppy's state, but not sure if that's much better.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that we can add an option remoteUploader to the plugins @uppy/aws-s3 and @uppy/aws-xhr-upload. then we don't need this bidirectional state dependency. So when using this feature you'd instead initialise plugins like this:

      uppyDashboard.use(AwsS3, { companionUrl: COMPANION_URL, remoteUploader: 'tus' })
      uppyDashboard.use(Transloadit, {
        waitForEncoding: true,
        importFromUploadURLs: true,
        assemblyOptions,
        onlyRemoteFiles: true,
      })

this will still allow people to customise the remote uploader (as needed in #5515)

Copy link
Member Author

Choose a reason for hiding this comment

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

imo remoteUploader: 'tus' and then installing Transloadit is very confusing and not worth it to make our code better.

Copy link
Contributor

Choose a reason for hiding this comment

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

this whole feature is extremely confusing, but the hidden state behavior makes it even more confusing compared to an option imo

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant I'm open to alternatives that do not involve yet another option besides this new option. If you can give me that we can talk about that more concretely. Two options that need to coordinate of which one is a confusing tus value while there is no tus in sight can't be the best way forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a tus uploader in sight though, inside companion - and that’s the one we’re using for remote files when this option is set, hence imo it makes sense to call a spade a spade and call the option remoteUploader: ‘tus’. It describes exactly what we’re doing here:
For remote files we use the uploader «tus».

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t have any better idea for which option to solve it, other than maybe an option on uppy core

Copy link
Member Author

Choose a reason for hiding this comment

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

Place yourself in the shoes of a user, they used @uppy/aws-s3 before and now want to use Transloadit-hosted companion. They install @uppy/transloadit and that's it, they don't care and may not know what Companion internals do, they just care that remote files end up at Transloadit. Therefor having to set not one, but two, options is error prone and the value of 'tus' is not expected. This will be confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that having to pass an option onlyRemoteFiles: true to Transloadit is equally confusing. But if we document it properly, they don't have to think much about whether there's 1 or 2 confusing options. And this option is not confusing for the people who want to customise the remote uploader being used, like in #5515

packages/@uppy/transloadit/src/index.ts Show resolved Hide resolved
packages/@uppy/transloadit/src/index.ts Show resolved Hide resolved
packages/@uppy/transloadit/src/index.ts Outdated Show resolved Hide resolved
* main: (38 commits)
  Release: [email protected] (#5617)
  @uppy/tus: fix resumeFromPreviousUpload race condition (#5616)
  @uppy/aws-s3: Fixed default shouldUseMultipart (#5613)
  build(deps): bump docker/build-push-action from 6.11.0 to 6.12.0 (#5611)
  @uppy/aws-s3: remove console.error (#5607)
  @uppy/companion: unify http error responses (#5595)
  Release: [email protected] (#5605)
  @uppy/aws-s3: always set S3 meta to UppyFile & include key (#5602)
  @uppy/companion: fix forcePathStyle boolean conversion (#5308)
  Fix Webpack CI (#5604)
  @uppy/aws-s3: allow uploads to fail/succeed independently (#5603)
  Revert "@uppy/aws-s3: allow uploads to fail/succeed independently"
  @uppy/aws-s3: allow uploads to fail/succeed independently
  Add types for css files (#5591)
  @uppy/unsplash: make utmSource optional (#5601)
  build(deps): bump docker/setup-qemu-action from 3.2.0 to 3.3.0 (#5599)
  build(deps): bump docker/build-push-action from 6.10.0 to 6.11.0 (#5600)
  @uppy/companion: add COMPANION_TUS_DEFERRED_UPLOAD_LENGTH (#5561)
  Release: [email protected] (#5590)
  Import types consistently from @uppy/core (#5589)
  ...
@Murderlon Murderlon requested a review from mifi January 23, 2025 13:15
packages/@uppy/xhr-upload/src/index.ts Show resolved Hide resolved
packages/@uppy/aws-s3/src/index.ts Show resolved Hide resolved
// Transloadit is only a pre and post processor.
// To let Transloadit hosted Companion download the file,
// we instruct any other upload plugin to use tus for remote uploads.
this.uppy.setState({ remoteUploader: 'tus' })
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that we can add an option remoteUploader to the plugins @uppy/aws-s3 and @uppy/aws-xhr-upload. then we don't need this bidirectional state dependency. So when using this feature you'd instead initialise plugins like this:

      uppyDashboard.use(AwsS3, { companionUrl: COMPANION_URL, remoteUploader: 'tus' })
      uppyDashboard.use(Transloadit, {
        waitForEncoding: true,
        importFromUploadURLs: true,
        assemblyOptions,
        onlyRemoteFiles: true,
      })

this will still allow people to customise the remote uploader (as needed in #5515)

packages/@uppy/xhr-upload/src/index.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants