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.

Sign up for GitHub

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
Open

Allow Transloadit-hosted Companion with other uploaders #5558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
22b183a
Allow Transloadit-hosted Companion with other uploaders
Murderlon Dec 19, 2024
5bec1c1
Merge branch 'main' into uploaders-with-hosted-companion
Murderlon Jan 23, 2025
9fa3f7d
Set tus properties more explicitly
Murderlon Jan 23, 2025
1bcd04c
Rename companionOnly -> onlyRemoteFiles
Murderlon Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/@uppy/aws-s3/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,9 +922,20 @@ export default class AwsS3Multipart<

// eslint-disable-next-line class-methods-use-this
#getCompanionClientArgs(file: UppyFile<M, B>) {
const opts = { ...this.opts }

// When you .use(AwsS3) with .use(Transloadit, { onlyRemoteFiles: true }),
// local files are uploaded with this plugin and remote files with the Transloadit plugin.
// Since the Transloadit plugin uses the tus plugin underneath, it's possible to have file.tus
// even though we are in this plugin.
// @ts-expect-error typed in @uppy/tus
const tusOpts = file.tus

return {
...file.remote?.body,
protocol: 's3-multipart',
protocol: this.uppy.getState().remoteUploader || 's3-multipart',
endpoint: tusOpts.endpoint ?? opts.endpoint,
headers: { ...opts.headers, ...tusOpts.headers },
size: file.data.size,
metadata: file.meta,
}
Expand Down
1 change: 1 addition & 0 deletions packages/@uppy/core/src/Uppy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ export interface State<M extends Meta, B extends Body>
currentUploads: Record<string, CurrentUpload<M, B>>
allowNewUpload: boolean
recoveredState: null | Required<Pick<State<M, B>, 'files' | 'currentUploads'>>
remoteUploader?: 'tus' | 's3-multipart' | 'multipart'
error: string | null
files: {
[key: string]: UppyFile<M, B>
Expand Down
35 changes: 26 additions & 9 deletions packages/@uppy/transloadit/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ export interface TransloaditOptions<M extends Meta, B extends Body>
waitForMetadata?: boolean
importFromUploadURLs?: boolean
alwaysRunAssembly?: boolean
/**
* Only use Transloadit for remote file uploads (such as from Google Drive).
* Enabling this means you have to install another plugin for local files,
* such as @uppy/aws-s3 or @uppy/xhr-upload.
*/
onlyRemoteFiles?: boolean
limit?: number
clientName?: string | null
retryDelays?: number[]
Expand All @@ -150,6 +156,7 @@ const defaultOptions = {
waitForMetadata: false,
alwaysRunAssembly: false,
importFromUploadURLs: false,
onlyRemoteFiles: false,
limit: 20,
retryDelays: [7_000, 10_000, 15_000, 20_000],
clientName: null,
Expand Down Expand Up @@ -300,6 +307,13 @@ export default class Transloadit<

this.i18nInit()

if (this.opts.onlyRemoteFiles) {
// 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

}

this.client = new Client({
service: this.opts.service,
client: this.#getClientVersion(),
Expand Down Expand Up @@ -807,26 +821,30 @@ export default class Transloadit<

assemblyOptions.fields ??= {}
validateParams(assemblyOptions.params)
const ids =
this.opts.onlyRemoteFiles ?
fileIDs.filter((id) => this.uppy.getFile(id).isRemote)
: fileIDs

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(ids, assemblyOptions))

if (assembly == null)
throw new Error('All files were canceled after assembly was created')

if (this.opts.importFromUploadURLs) {
await this.#reserveFiles(assembly, fileIDs)
await this.#reserveFiles(assembly, ids)
}
fileIDs.forEach((fileID) => {
ids.forEach((fileID) => {
const file = this.uppy.getFile(fileID)
this.uppy.emit('preprocess-complete', file)
})
this.#createAssemblyWatcher(assembly.status.assembly_id)
this.#connectAssembly(assembly, fileIDs)
this.#connectAssembly(assembly, ids)
} catch (err) {
fileIDs.forEach((fileID) => {
ids.forEach((fileID) => {
const file = this.uppy.getFile(fileID)
// Clear preprocessing state when the Assembly could not be created,
// otherwise the UI gets confused about the lingering progress keys
Expand Down Expand Up @@ -942,9 +960,9 @@ export default class Transloadit<
if (this.opts.importFromUploadURLs) {
// No uploader needed when importing; instead we take the upload URL from an existing uploader.
this.uppy.on('upload-success', this.#onFileUploadURLAvailable)
} else {
// we don't need it here.
// the regional endpoint from the Transloadit API before we can set it.
// If onlyRemoteFiles is true, another uploader plugin is installed for local uploads
// and we only use Transloadit to create an assembly for the remote files.
} else if (!this.opts.onlyRemoteFiles) {
this.uppy.use(Tus, {
// Disable tus-js-client fingerprinting, otherwise uploading the same file at different times
// will upload to an outdated Assembly, and we won't get socket events for it.
Expand All @@ -959,7 +977,6 @@ export default class Transloadit<
// Send all metadata to Transloadit. Metadata set by the user
// ends up as in the template as `file.user_meta`
allowedMetaFields: true,
// Pass the limit option to @uppy/tus
limit: this.opts.limit,
rateLimitedQueue: this.#rateLimitedQueue,
retryDelays: this.opts.retryDelays,
Expand Down
13 changes: 10 additions & 3 deletions packages/@uppy/xhr-upload/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,18 +440,25 @@ export default class XHRUpload<
opts.allowedMetaFields,
file.meta,
)
// When you .use(XHR) with .use(Transloadit, { onlyRemoteFiles: true }),
// local files are uploaded with this plugin and remote files with the Transloadit plugin.
// Since the Transloadit plugin uses the tus plugin underneath, it's possible to have file.tus
// even though we are in this plugin.
// @ts-expect-error typed in @uppy/tus
const tusOpts = file.tus

return {
...file.remote?.body,
protocol: 'multipart',
endpoint: opts.endpoint,
protocol: this.uppy.getState().remoteUploader || 'multipart',
endpoint: 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,
}
}

Expand Down
12 changes: 11 additions & 1 deletion private/dev/Dashboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ export default () => {
.use(Webdav, {
target: Dashboard,
companionUrl: COMPANION_URL,
companionAllowedHosts
companionAllowedHosts,
})
.use(Audio, {
target: Dashboard,
Expand All @@ -195,6 +195,15 @@ export default () => {
shouldUseMultipart: false,
})
break
case 's3-with-transloadit-companion':
uppyDashboard.use(AwsS3, { endpoint: COMPANION_URL })
uppyDashboard.use(Transloadit, {
service: TRANSLOADIT_SERVICE_URL,
waitForEncoding: true,
assemblyOptions,
onlyRemoteFiles: true,
})
break
case 's3-multipart':
uppyDashboard.use(AwsS3, {
endpoint: COMPANION_URL,
Expand All @@ -221,6 +230,7 @@ export default () => {
waitForEncoding: true,
importFromUploadURLs: true,
assemblyOptions,
onlyRemoteFiles: true,
})
break
case 'transloadit-xhr':
Expand Down
Loading