@uppy/aws-s3: add `endpoint` option #5173

aduh95 · 2024-05-16T07:48:29Z

I don't really get why I'm getting TS errors though

github-actions · 2024-05-16T07:49:55Z

Diff output files

diff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index f114dba..efcb09b 100644
--- a/packages/@uppy/aws-s3/lib/index.js
+++ b/packages/@uppy/aws-s3/lib/index.js
@@ -60,10 +60,10 @@ const defaultOptions = {
   getTemporarySecurityCredentials: false,
   shouldUseMultipart: file => file.size >> 10 >> 10 > 100,
   retryDelays: [0, 1000, 3000, 5000],
-  companionHeaders: {},
 };
 var _companionCommunicationQueue = _classPrivateFieldLooseKey("companionCommunicationQueue");
 var _client = _classPrivateFieldLooseKey("client");
+var _setClient = _classPrivateFieldLooseKey("setClient");
 var _assertHost = _classPrivateFieldLooseKey("assertHost");
 var _cachedTemporaryCredentials = _classPrivateFieldLooseKey("cachedTemporaryCredentials");
 var _getTemporarySecurityCredentials = _classPrivateFieldLooseKey("getTemporarySecurityCredentials");
@@ -76,8 +76,8 @@ var _setCompanionHeaders = _classPrivateFieldLooseKey("setCompanionHeaders");
 var _setResumableUploadsCapability = _classPrivateFieldLooseKey("setResumableUploadsCapability");
 var _resetResumableCapability = _classPrivateFieldLooseKey("resetResumableCapability");
 export default class AwsS3Multipart extends BasePlugin {
-  constructor(uppy, opts) {
-    var _ref3, _rateLimitedQueue;
+  constructor(uppy, _opts) {
+    var _rateLimitedQueue;
     super(uppy, {
       ...defaultOptions,
       uploadPartBytes: AwsS3Multipart.uploadPartBytes,
@@ -87,7 +87,7 @@ export default class AwsS3Multipart extends BasePlugin {
       completeMultipartUpload: null,
       signPart: null,
       getUploadParameters: null,
-      ...opts,
+      ..._opts,
     });
     Object.defineProperty(this, _getCompanionClientArgs, {
       value: _getCompanionClientArgs2,
@@ -101,6 +101,9 @@ export default class AwsS3Multipart extends BasePlugin {
     Object.defineProperty(this, _assertHost, {
       value: _assertHost2,
     });
+    Object.defineProperty(this, _setClient, {
+      value: _setClient2,
+    });
     Object.defineProperty(this, _companionCommunicationQueue, {
       writable: true,
       value: void 0,
@@ -181,7 +184,9 @@ export default class AwsS3Multipart extends BasePlugin {
     Object.defineProperty(this, _setCompanionHeaders, {
       writable: true,
       value: () => {
-        _classPrivateFieldLooseBase(this, _client)[_client].setCompanionHeaders(this.opts.companionHeaders);
+        var _classPrivateFieldLoo;
+        (_classPrivateFieldLoo = _classPrivateFieldLooseBase(this, _client)[_client]) == null
+          || _classPrivateFieldLoo.setCompanionHeaders(this.opts.headers);
       },
     });
     Object.defineProperty(this, _setResumableUploadsCapability, {
@@ -206,14 +211,14 @@ export default class AwsS3Multipart extends BasePlugin {
     });
     this.type = "uploader";
     this.id = this.opts.id || "AwsS3Multipart";
-    _classPrivateFieldLooseBase(this, _client)[_client] = new RequestClient(uppy, (_ref3 = opts) != null ? _ref3 : {});
+    _classPrivateFieldLooseBase(this, _setClient)[_setClient](_opts);
     const dynamicDefaultOptions = {
       createMultipartUpload: this.createMultipartUpload,
       listParts: this.listParts,
       abortMultipartUpload: this.abortMultipartUpload,
       completeMultipartUpload: this.completeMultipartUpload,
-      signPart: opts != null && opts.getTemporarySecurityCredentials ? this.createSignedURL : this.signPart,
-      getUploadParameters: opts != null && opts.getTemporarySecurityCredentials
+      signPart: _opts != null && _opts.getTemporarySecurityCredentials ? this.createSignedURL : this.signPart,
+      getUploadParameters: _opts != null && _opts.getTemporarySecurityCredentials
         ? this.createSignedURL
         : this.getUploadParameters,
     };
@@ -243,7 +248,7 @@ export default class AwsS3Multipart extends BasePlugin {
       newOptions,
     );
     super.setOptions(newOptions);
-    _classPrivateFieldLooseBase(this, _setCompanionHeaders)[_setCompanionHeaders]();
+    _classPrivateFieldLooseBase(this, _setClient)[_setClient](newOptions);
   }
   resetUploaderReferences(fileID, opts) {
     if (this.uploaders[fileID]) {
@@ -273,13 +278,13 @@ export default class AwsS3Multipart extends BasePlugin {
       signal,
     }).then(assertServerError);
   }
-  listParts(file, _ref4, oldSignal) {
+  listParts(file, _ref3, oldSignal) {
     var _signal;
     let {
       key,
       uploadId,
       signal,
-    } = _ref4;
+    } = _ref3;
     (_signal = signal) != null ? _signal : signal = oldSignal;
     _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("listParts");
     throwIfAborted(signal);
@@ -291,14 +296,14 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     ).then(assertServerError);
   }
-  completeMultipartUpload(file, _ref5, oldSignal) {
+  completeMultipartUpload(file, _ref4, oldSignal) {
     var _signal2;
     let {
       key,
       uploadId,
       parts,
       signal,
-    } = _ref5;
+    } = _ref4;
     (_signal2 = signal) != null ? _signal2 : signal = oldSignal;
     _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("completeMultipartUpload");
     throwIfAborted(signal);
@@ -343,13 +348,13 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     };
   }
-  signPart(file, _ref6) {
+  signPart(file, _ref5) {
     let {
       uploadId,
       key,
       partNumber,
       signal,
-    } = _ref6;
+    } = _ref5;
     _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("signPart");
     throwIfAborted(signal);
     if (uploadId == null || key == null || partNumber == null) {
@@ -363,12 +368,12 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     ).then(assertServerError);
   }
-  abortMultipartUpload(file, _ref7) {
+  abortMultipartUpload(file, _ref6) {
     let {
       key,
       uploadId,
       signal,
-    } = _ref7;
+    } = _ref6;
     _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("abortMultipartUpload");
     const filename = encodeURIComponent(key);
     const uploadIdEnc = encodeURIComponent(uploadId);
@@ -401,7 +406,7 @@ export default class AwsS3Multipart extends BasePlugin {
     });
     return _classPrivateFieldLooseBase(this, _client)[_client].get(`s3/params?${query}`, options);
   }
-  static async uploadPartBytes(_ref8) {
+  static async uploadPartBytes(_ref7) {
     let {
       signature: {
         url,
@@ -414,7 +419,7 @@ export default class AwsS3Multipart extends BasePlugin {
       onProgress,
       onComplete,
       signal,
-    } = _ref8;
+    } = _ref7;
     throwIfAborted(signal);
     if (url == null) {
       throw new Error("Cannot upload to an undefined URL");
@@ -528,23 +533,61 @@ export default class AwsS3Multipart extends BasePlugin {
     );
   }
 }
+function _setClient2(opts) {
+  if (
+    opts == null
+    || !("endpoint" in opts || "companionUrl" in opts || "headers" in opts || "companionHeaders" in opts
+      || "cookiesRule" in opts || "companionCookiesRule" in opts)
+  ) return;
+  if ("companionUrl" in opts && !("endpoint" in opts)) {
+    this.uppy.log("`companionUrl` option has been removed in @uppy/aws-s3, use `endpoint` instead.", "warning");
+  }
+  if ("companionHeaders" in opts && !("headers" in opts)) {
+    this.uppy.log("`companionHeaders` option has been removed in @uppy/aws-s3, use `headers` instead.", "warning");
+  }
+  if ("companionCookiesRule" in opts && !("cookiesRule" in opts)) {
+    this.uppy.log(
+      "`companionCookiesRule` option has been removed in @uppy/aws-s3, use `cookiesRule` instead.",
+      "warning",
+    );
+  }
+  if ("endpoint" in opts) {
+    _classPrivateFieldLooseBase(this, _client)[_client] = new RequestClient(this.uppy, {
+      pluginId: this.id,
+      provider: "AWS",
+      companionUrl: this.opts.endpoint,
+      companionHeaders: this.opts.headers,
+      companionCookiesRule: this.opts.cookiesRule,
+    });
+  } else {
+    if ("headers" in opts) {
+      _classPrivateFieldLooseBase(this, _setCompanionHeaders)[_setCompanionHeaders]();
+    }
+    if ("cookiesRule" in opts) {
+      _classPrivateFieldLooseBase(this, _client)[_client].opts.companionCookiesRule = opts.cookiesRule;
+    }
+  }
+}
 function _assertHost2(method) {
-  if (!this.opts.companionUrl) {
+  if (!_classPrivateFieldLooseBase(this, _client)[_client]) {
     throw new Error(
-      `Expected a \`companionUrl\` option containing a Companion address, or if you are not using Companion, a custom \`${method}\` implementation.`,
+      `Expected a \`endpoint\` option containing a URL, or if you are not using Companion, a custom \`${method}\` implementation.`,
     );
   }
 }
 async function _getTemporarySecurityCredentials2(options) {
   throwIfAborted(options == null ? void 0 : options.signal);
   if (_classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials] == null) {
-    if (this.opts.getTemporarySecurityCredentials === true) {
+    const {
+      getTemporarySecurityCredentials,
+    } = this.opts;
+    if (getTemporarySecurityCredentials === true) {
       _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("getTemporarySecurityCredentials");
       _classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials] =
         _classPrivateFieldLooseBase(this, _client)[_client].get("s3/sts", options).then(assertServerError);
     } else {
-      _classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials] = this.opts
-        .getTemporarySecurityCredentials(options);
+      _classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials] =
+        getTemporarySecurityCredentials(options);
     }
     _classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials] =
       await _classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials];

packages/@uppy/aws-s3/src/index.ts

Murderlon · 2024-05-16T08:57:32Z

packages/@uppy/aws-s3/src/index.ts

-  companionUrl: string
+type AWSS3WithCompanion = (
+  | {
+      /** @deprecated use `endpoint` instead */


I'm okay with doing a breaking change. If not now then when?

On the next major

Murderlon · 2024-05-30T08:47:25Z

Still want to get this over the finish line?

Things needed:

Good docs on how to structure your backend endpoints so you don't have to write any client-side logic (match companion server-side)
In my opinion, we should remove, not deprecate. We're doing a major now, people have to go through a migration guide anyway.
@uppy/aws-s3: add endpoint option #5173 (comment)

Murderlon

Looks good. I think it's still important to have:

New ## Examples section with ### Setting up your backend, which explains what you actually need to do to match Companion.
Link to that from the endpoint option
Link to that from the migration guide

Murderlon · 2024-06-10T07:55:53Z

docs/guides/migration-guides.md

@@ -199,6 +199,19 @@ uppy.use(Dropbox, {

 ### `@uppy/aws-s3-multipart`

+#### Companion’s options (`companionUrl`, `companionHeaders`, and `companionCookieRules`) are renamed to more generic names (`endpoint`, `headers`, and `cookieRules`)


This is placed inside the 2.x to 3.x docs. You can move it correctly when #5206 is merged.

examples/aws-nodejs/index.js

aduh95 · 2024-06-11T16:04:33Z

examples/aws-nodejs/index.js

@@ -108,7 +109,11 @@ app.get('/sts', (req, res, next) => {
    })
  }, next)
 })
-app.post('/sign-s3', (req, res, next) => {
+app.get('/s3/params', (req, res, next) => {


I wonder if it would be worth renaming the Companion endpoint /s3/params is much less clear than /sign-s3 IMO

Unfortunately Companion always has to be backwards compatible as old Uppy clients might connect to Transloadit. So I'm a little more conservative with renames and such.

I guess we could support both

I don't think we can ever break this api (or else we cannot upgrade transloadit's companion server without breaking clients). i don't want to indefinitely have two endpoints doing the same thing with different names.

Murderlon

All feedback nicely addressed 👌

Good to go after input from Mikael on my last comment below and we have to merge #5206 so we can correctly integrate your migration points in there.

Murderlon · 2024-06-12T07:42:06Z

packages/@uppy/aws-s3/src/index.ts

@@ -385,10 +386,59 @@ export default class AwsS3Multipart<
    return this.#client
  }

+  #setClient(opts?: Partial<AwsS3MultipartOptions<M, B>>) {


I still think this is a lot of code for backwards compatibility which we're not obliged to do. People can not blindly upgrade Uppy anyway, so you have to already check all things of the migration guide anyway. Why not just errors in the constructor, saying the properties are renamed?

Maybe we should let @mifi settle it, backwards compatible or breaking for 4.x.

we are breaking many other apis in 4.x, so why not break this too?

This does not have any backward compatiblity, it's just emitting warnings if folks are using the old option names.

packages/@uppy/aws-s3/src/index.ts

mifi · 2024-06-12T13:19:15Z

packages/@uppy/aws-s3/src/index.ts

+          // @ts-expect-error It's either a function or `false`, which should error.
+          getTemporarySecurityCredentials(options)


Suggested change

// @ts-expect-error It's either a function or `false`, which should error.

getTemporarySecurityCredentials(options)

if (typeof getTemporarySecurityCredentials !== 'function') throw new Error('Expected getTemporarySecurityCredentials to be "true" or a function')

getTemporarySecurityCredentials(options)

better to check for it. @ts-expect-error will blanket disable all errors. e.g. a wrong options type.

This expression is not callable. Type 'never' has no call signatures.

is getTemporarySecurityCredentials never? is it possible to fix the types of getTemporarySecurityCredentials so that it's not never?

I replaced ts-expect-error with a as, so we're still validating options type.

mifi · 2024-06-12T13:23:43Z

examples/aws-nodejs/index.js

@@ -108,7 +109,11 @@ app.get('/sts', (req, res, next) => {
    })
  }, next)
 })
-app.post('/sign-s3', (req, res, next) => {
+app.get('/s3/params', (req, res, next) => {


I don't think we can ever break this api (or else we cannot upgrade transloadit's companion server without breaking clients). i don't want to indefinitely have two endpoints doing the same thing with different names.

mifi · 2024-06-12T13:25:00Z

packages/@uppy/aws-s3/src/index.ts

@@ -385,10 +386,59 @@ export default class AwsS3Multipart<
    return this.#client
  }

+  #setClient(opts?: Partial<AwsS3MultipartOptions<M, B>>) {


we are breaking many other apis in 4.x, so why not break this too?

Murderlon

LGTM after two last suggestions

docs/guides/migration-guides.md

docs/uploader/aws-s3-multipart.mdx

Co-authored-by: Merlijn Vos <[email protected]>

* 4.x: Renames & `eslint-disable react/require-default-props` removal (#5251) coalesce options `bucket` and `getKey` (#5169) @uppy/aws-s3: add `endpoint` option (#5173) @uppy/locales: fix `fa_IR` export (#5241) improve companion logging (#5250) Release: [email protected] (#5243) @uppy/core: add generic to `getPlugin` (#5247) docs: add 4.x migration guide (#5206) @uppy/transloadit: also fix outdated assembly transloadit:result (#5246) docs - fix typo in the url @uppy/core: set default for Body generic (#5244) Release: [email protected] (#5242) docs: clarify assemblyOptions for @uppy/transloadit (#5226) meta: Improve aws-node example readme (#4753) @uppy/react: remove `react:` prefix from `id` & allow `id` as a prop (#5228) Added translation string (it_IT) (#5237) docs: correct allowedMetaFields (#5227) @uppy/transloadit: fix transloadit:result event (#5231) docs: remove `extraData` note from migration guide (#5219) @uppy/provider-views: fix wrong font for files (#5234)

| Package | Version | Package | Version | | -------------------- | ------------- | -------------------- | ------------- | | @uppy/aws-s3 | 4.0.0-beta.7 | @uppy/locales | 4.0.0-beta.4 | | @uppy/box | 3.0.0-beta.7 | @uppy/onedrive | 4.0.0-beta.7 | | @uppy/companion | 5.0.0-beta.10 | @uppy/provider-views | 4.0.0-beta.9 | | @uppy/core | 4.0.0-beta.10 | @uppy/react | 4.0.0-beta.7 | | @uppy/dashboard | 4.0.0-beta.10 | @uppy/remote-sources | 2.0.0-beta.5 | | @uppy/dropbox | 4.0.0-beta.8 | @uppy/transloadit | 4.0.0-beta.9 | | @uppy/google-drive | 3.6.0-beta.1 | uppy | 4.0.0-beta.12 | | @uppy/google-photos | 0.2.0-beta.1 | | | - meta: ignore `require-default-props` lint rule for function components (Antoine du Hamel / #5253) - @uppy/provider-views: Renames & `eslint-disable react/require-default-props` removal (Evgenia Karunus / #5251) - @uppy/companion: coalesce options `bucket` and `getKey` (Mikael Finstad / #5169) - @uppy/aws-s3: add `endpoint` option (Antoine du Hamel / #5173) - @uppy/locales: fix `fa_IR` export (Merlijn Vos / #5241) - @uppy/companion: improve companion logging (Mikael Finstad / #5250) - @uppy/transloadit: also fix outdated assembly transloadit:result (Merlijn Vos / #5246) - docs: fix typo in the url (Evgenia Karunus) - examples,@uppy/locales,@uppy/provider-views,@uppy/transloadit: Release: [email protected] (github-actions[bot] / #5242) - meta: Improve aws-node example readme (Artur Paikin / #4753) - @uppy/locales: Added translation string (it_IT) (Samuel / #5237) - @uppy/transloadit: fix transloadit:result event (Merlijn Vos / #5231) - @uppy/provider-views: fix wrong font for files (Merlijn Vos / #5234)

@uppy/aws-s3: add endpoint option

07f17e4

aduh95 added 4 commits May 16, 2024 10:22

fix tests

dfbd00d

prettier

971296c

doc: deprecated option

9369e7e

add deprecation warning

5a19fc9

Murderlon reviewed May 16, 2024

View reviewed changes

Murderlon linked an issue May 30, 2024 that may be closed by this pull request

When using aws-s3, force endpoints to be the same as Companion to simplify setup #5030

Closed

2 tasks

aduh95 added 4 commits June 7, 2024 16:22

Merge branch '4.x' of https://github.com/transloadit/uppy

c9798bd

apply suggestions

02beb51

avoid double setting of headers

35525ae

fix e2e and add migration note

1686925

Murderlon reviewed Jun 10, 2024

View reviewed changes

aduh95 added 2 commits June 10, 2024 20:28

Merge branch '4.x' of https://github.com/transloadit/uppy

85278ec

add example

f4dff7c

Murderlon reviewed Jun 11, 2024

View reviewed changes

examples/aws-nodejs/index.js Outdated Show resolved Hide resolved

Apply feedback

84deafc

aduh95 commented Jun 11, 2024

View reviewed changes

aduh95 added 2 commits June 11, 2024 18:05

prettier

6a61f67

fix custom endpoint

e6f88c4

Murderlon reviewed Jun 12, 2024

View reviewed changes

mifi reviewed Jun 12, 2024

View reviewed changes

aduh95 and others added 3 commits June 12, 2024 17:33

Merge branch '4.x' into aws-companion-endpoint

de54ce7

Merge branch '4.x' into aws-companion-endpoint

25680cd

move migration guide

a319f42

Murderlon approved these changes Jun 13, 2024

View reviewed changes

docs/guides/migration-guides.md Outdated Show resolved Hide resolved

docs/uploader/aws-s3-multipart.mdx Outdated Show resolved Hide resolved

aduh95 and others added 3 commits June 13, 2024 12:09

Apply suggestions from code review

84883e4

Co-authored-by: Merlijn Vos <[email protected]>

prettier

f396841

Merge branch '4.x' of https://github.com/transloadit/uppy

21d1e1c

aduh95 added 3 commits June 13, 2024 14:38

fixup! Merge branch '4.x' of https://github.com/transloadit/uppy

8b9dcd9

check for WebCrypto

85dea61

remove @ts-expect-error

88f47fa

aduh95 merged commit 087d8f0 into 4.x Jun 13, 2024
17 checks passed

aduh95 deleted the aws-companion-endpoint branch June 13, 2024 12:55

github-actions bot mentioned this pull request Jun 18, 2024

Release: [email protected] #5259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@uppy/aws-s3: add `endpoint` option #5173

@uppy/aws-s3: add `endpoint` option #5173

aduh95 commented May 16, 2024

github-actions bot commented May 16, 2024 •

edited

Loading

Murderlon May 16, 2024

aduh95 May 16, 2024

Murderlon commented May 30, 2024

Murderlon left a comment

Murderlon Jun 10, 2024

aduh95 Jun 11, 2024

Murderlon Jun 12, 2024

aduh95 Jun 12, 2024

mifi Jun 12, 2024

Murderlon left a comment

Murderlon Jun 12, 2024

mifi Jun 12, 2024

aduh95 Jun 12, 2024

mifi Jun 12, 2024

aduh95 Jun 12, 2024

mifi Jun 12, 2024

aduh95 Jun 13, 2024

mifi Jun 12, 2024

mifi Jun 12, 2024

Murderlon left a comment

		@@ -199,6 +199,19 @@ uppy.use(Dropbox, {

		### `@uppy/aws-s3-multipart`

		#### Companion’s options (`companionUrl`, `companionHeaders`, and `companionCookieRules`) are renamed to more generic names (`endpoint`, `headers`, and `cookieRules`)

		// @ts-expect-error It's either a function or `false`, which should error.
		getTemporarySecurityCredentials(options)

@uppy/aws-s3: add endpoint option #5173

@uppy/aws-s3: add endpoint option #5173

Conversation

aduh95 commented May 16, 2024

github-actions bot commented May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murderlon commented May 30, 2024

Murderlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murderlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murderlon left a comment

Choose a reason for hiding this comment

@uppy/aws-s3: add `endpoint` option #5173

@uppy/aws-s3: add `endpoint` option #5173

github-actions bot commented May 16, 2024 •

edited

Loading