-
Notifications
You must be signed in to change notification settings - Fork 2k
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/aws-s3-multipart: add support for signing on the client #4519
Conversation
.yarn/patches/jest-environment-jsdom-npm-29.5.0-fc600add1e.patch
Outdated
Show resolved
Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
@@ -25,6 +34,8 @@ export interface AwsS3MultipartOptions extends PluginOptions { | |||
file: UppyFile, | |||
opts: { uploadId: string; key: string; signal: AbortSignal } | |||
) => MaybePromise<AwsS3Part[]> | |||
getTemporarySecurityCredentials?: boolean | (() => MaybePromise<AwsS3STSResponse>) | |||
getTemporarySecurityCredentialsExpiry?: (credentials: AwsS3STSResponse['credentials'], options?: {signal?: AbortSignal}) => MaybePromise<number> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could type this more strictly to only allow boolean
if companionUrl
is set. It's a bit more involved but here is an example:
uppy/packages/@uppy/transloadit/types/index.d.ts
Lines 116 to 153 in 7a90f9b
export type TransloaditOptions = Options & | |
( | |
| { | |
assemblyOptions?: AssemblyOptions | ((file?: UppyFile) => Promise<AssemblyOptions> | AssemblyOptions) | |
/** @deprecated use `assemblyOptions` instead */ | |
getAssemblyOptions?: never | |
/** @deprecated use `assemblyOptions` instead */ | |
params?: never | |
/** @deprecated use `assemblyOptions` instead */ | |
fields?: never | |
/** @deprecated use `assemblyOptions` instead */ | |
signature?: never | |
} | |
| { | |
/** @deprecated use `assemblyOptions` instead */ | |
getAssemblyOptions?: ( | |
file?: UppyFile | |
) => AssemblyOptions | Promise<AssemblyOptions> | |
assemblyOptions?: never | |
/** @deprecated use `assemblyOptions` instead */ | |
params?: never | |
/** @deprecated use `assemblyOptions` instead */ | |
fields?: never | |
/** @deprecated use `assemblyOptions` instead */ | |
signature?: never | |
} | |
| { | |
/** @deprecated use `assemblyOptions` instead */ | |
params?: AssemblyParameters | |
/** @deprecated use `assemblyOptions` instead */ | |
fields?: { [name: string]: number | string } | string[] | |
/** @deprecated use `assemblyOptions` instead */ | |
signature?: string | |
/** @deprecated use `assemblyOptions` instead */ | |
getAssemblyOptions?: never | |
assemblyOptions?: never | |
} | |
) |
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking, I think it should go on its own PR, because the other options (e.g. signPart
, abortMultipartUpload
, etc.) would also benefit for more fine-grained typings.
.yarn/patches/jest-environment-jsdom-npm-29.5.0-fc600add1e.patch
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good!
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3-multipart | 3.5.0 | @uppy/locales | 3.2.3 | | @uppy/box | 2.1.2 | @uppy/onedrive | 3.1.2 | | @uppy/companion | 4.7.0 | @uppy/provider-views | 3.4.0 | | @uppy/companion-client | 3.2.1 | @uppy/react | 3.1.3 | | @uppy/core | 3.3.1 | @uppy/status-bar | 3.2.2 | | @uppy/dashboard | 3.4.2 | @uppy/transloadit | 3.2.0 | | @uppy/dropbox | 3.1.2 | @uppy/utils | 5.4.1 | | @uppy/google-drive | 3.2.0 | uppy | 3.12.0 | - @uppy/transloadit: fix error message (Antoine du Hamel / #4572) - @uppy/provider-views: add support for remote file paths (Mikael Finstad / #4537) - @uppy/transloadit: implement Server-sent event API (Antoine du Hamel / #4098) - @uppy/aws-s3-multipart: add support for signing on the client (Antoine du Hamel / #4519) - @uppy/react: allow `id` from props (Merlijn Vos / #4570) - @uppy/aws-s3-multipart: fix lint warning (Antoine du Hamel / #4569) - @uppy/status-bar: listen to `upload` event instead of button click (Antoine du Hamel / #4563) - @uppy/aws-s3-multipart: fix support for non-multipart PUT upload (Antoine du Hamel / #4568) - @uppy/companion: fix esm imports in production/transpiled builds (Dominik Schmidt / #4561) - @uppy/locales: fix expression and spelling errors in es_ES (Rubén / #4567) - meta: upgrade dev dependencies (dependabot\[bot\]) - meta: Don't use triage label (Artur Paikin / #4552) - meta: update Cypress (Antoine du Hamel / #4562) - @uppy/box,@uppy/companion,@uppy/dropbox,@uppy/google-drive,@uppy/onedrive,@uppy/provider-views: Load Google Drive / OneDrive lists 5-10x faster & always load all files (Merlijn Vos / #4513) - @uppy/locales: Add missing pt-BR locales for ImageEditor plugin (Mateus Cruz / #4558)
This adds opt-in support for signing on the client (the default is on the server). This works for both multipart and non-multipart uploads.
Implementers should opt-in into this only if they are confortable giving end users direct access to signing files for their bucket. It decreases by about 20% the upload for both multipart and non-multipart.
We might want to make the Companion changes more configurable, however it can happen in a follow up PR.