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

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

Open
wants to merge 5 commits into
base: 4.x
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

aduh95
Copy link
Member

@aduh95 aduh95 commented May 16, 2024

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

Copy link
Contributor

github-actions bot commented May 16, 2024

Diff output files
diff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index bcff951..2137896 100644
--- a/packages/@uppy/aws-s3/lib/index.js
+++ b/packages/@uppy/aws-s3/lib/index.js
@@ -66,6 +66,8 @@ const defaultOptions = {
 };
 var _companionCommunicationQueue = _classPrivateFieldLooseKey("companionCommunicationQueue");
 var _client = _classPrivateFieldLooseKey("client");
+var _setClient = _classPrivateFieldLooseKey("setClient");
+var _assertHost = _classPrivateFieldLooseKey("assertHost");
 var _cachedTemporaryCredentials = _classPrivateFieldLooseKey("cachedTemporaryCredentials");
 var _getTemporarySecurityCredentials = _classPrivateFieldLooseKey("getTemporarySecurityCredentials");
 var _setS3MultipartState = _classPrivateFieldLooseKey("setS3MultipartState");
@@ -77,7 +79,7 @@ var _setCompanionHeaders = _classPrivateFieldLooseKey("setCompanionHeaders");
 var _setResumableUploadsCapability = _classPrivateFieldLooseKey("setResumableUploadsCapability");
 var _resetResumableCapability = _classPrivateFieldLooseKey("resetResumableCapability");
 export default class AwsS3Multipart extends BasePlugin {
-  constructor(uppy, opts) {
+  constructor(uppy, _opts) {
     var _rateLimitedQueue;
     super(uppy, {
       ...defaultOptions,
@@ -88,7 +90,7 @@ export default class AwsS3Multipart extends BasePlugin {
       completeMultipartUpload: null,
       signPart: null,
       getUploadParameters: null,
-      ...opts,
+      ..._opts,
     });
     Object.defineProperty(this, _getCompanionClientArgs, {
       value: _getCompanionClientArgs2,
@@ -99,6 +101,12 @@ export default class AwsS3Multipart extends BasePlugin {
     Object.defineProperty(this, _getTemporarySecurityCredentials, {
       value: _getTemporarySecurityCredentials2,
     });
+    Object.defineProperty(this, _assertHost, {
+      value: _assertHost2,
+    });
+    Object.defineProperty(this, _setClient, {
+      value: _setClient2,
+    });
     Object.defineProperty(this, _companionCommunicationQueue, {
       writable: true,
       value: void 0,
@@ -179,7 +187,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.companionHeaders);
       },
     });
     Object.defineProperty(this, _setResumableUploadsCapability, {
@@ -204,14 +214,14 @@ export default class AwsS3Multipart extends BasePlugin {
     });
     this.type = "uploader";
     this.id = this.opts.id || "AwsS3Multipart";
-    _classPrivateFieldLooseBase(this, _client)[_client] = new RequestClient(uppy, opts);
+    _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,
     };
@@ -242,6 +252,7 @@ export default class AwsS3Multipart extends BasePlugin {
       newOptions,
     );
     super.setOptions(newOptions);
+    _classPrivateFieldLooseBase(this, _setClient)[_setClient](newOptions);
     _classPrivateFieldLooseBase(this, _setCompanionHeaders)[_setCompanionHeaders]();
   }
   resetUploaderReferences(fileID, opts) {
@@ -260,15 +271,8 @@ export default class AwsS3Multipart extends BasePlugin {
       this.uploaderSockets[fileID] = null;
     }
   }
-  assertHost(method) {
-    if (!this.opts.companionUrl) {
-      throw new Error(
-        `Expected a \`companionUrl\` option containing a Companion address, or if you are not using Companion, a custom \`${method}\` implementation.`,
-      );
-    }
-  }
   createMultipartUpload(file, signal) {
-    this.assertHost("createMultipartUpload");
+    _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("createMultipartUpload");
     throwIfAborted(signal);
     const allowedMetaFields = getAllowedMetaFields(this.opts.allowedMetaFields, file.meta);
     const metadata = getAllowedMetadata({
@@ -291,7 +295,7 @@ export default class AwsS3Multipart extends BasePlugin {
       signal,
     } = _ref3;
     (_signal = signal) != null ? _signal : signal = oldSignal;
-    this.assertHost("listParts");
+    _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("listParts");
     throwIfAborted(signal);
     const filename = encodeURIComponent(key);
     return _classPrivateFieldLooseBase(this, _client)[_client].get(`s3/multipart/${uploadId}?key=${filename}`, {
@@ -307,7 +311,7 @@ export default class AwsS3Multipart extends BasePlugin {
       signal,
     } = _ref4;
     (_signal2 = signal) != null ? _signal2 : signal = oldSignal;
-    this.assertHost("completeMultipartUpload");
+    _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("completeMultipartUpload");
     throwIfAborted(signal);
     const filename = encodeURIComponent(key);
     const uploadIdEnc = encodeURIComponent(uploadId);
@@ -357,7 +361,7 @@ export default class AwsS3Multipart extends BasePlugin {
       partNumber,
       signal,
     } = _ref5;
-    this.assertHost("signPart");
+    _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("signPart");
     throwIfAborted(signal);
     if (uploadId == null || key == null || partNumber == null) {
       throw new Error("Cannot sign without a key, an uploadId, and a partNumber");
@@ -378,7 +382,7 @@ export default class AwsS3Multipart extends BasePlugin {
       signal,
     } = _ref6;
     (_signal3 = signal) != null ? _signal3 : signal = oldSignal;
-    this.assertHost("abortMultipartUpload");
+    _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("abortMultipartUpload");
     const filename = encodeURIComponent(key);
     const uploadIdEnc = encodeURIComponent(uploadId);
     return _classPrivateFieldLooseBase(this, _client)[_client].delete(
@@ -529,11 +533,35 @@ export default class AwsS3Multipart extends BasePlugin {
     );
   }
 }
+function _setClient2(opts) {
+  if (
+    opts == null
+    || !("endpoint" in opts || "companionUrl" in opts || "companionHeaders" in opts || "companionCookiesRule" in opts
+      || "companionKeysParams" in opts)
+  ) return;
+  if (opts.endpoint) {
+    opts = {
+      ...this.opts,
+      companionUrl: opts.endpoint,
+    };
+  } else if (opts.companionUrl) {
+    this.uppy.log("`companionUrl` option is deprecated in @uppy/aws-s3, use `endpoint` instead.", "warning");
+    opts = this.opts;
+  }
+  _classPrivateFieldLooseBase(this, _client)[_client] = new RequestClient(this.uppy, opts);
+}
+function _assertHost2(method) {
+  if (!_classPrivateFieldLooseBase(this, _client)[_client]) {
+    throw new Error(
+      `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) {
-      this.assertHost("getTemporarySecurityCredentials");
+      _classPrivateFieldLooseBase(this, _assertHost)[_assertHost]("getTemporarySecurityCredentials");
       _classPrivateFieldLooseBase(this, _cachedTemporaryCredentials)[_cachedTemporaryCredentials] =
         _classPrivateFieldLooseBase(this, _client)[_client].get("s3/sts", options).then(assertServerError);
     } else {

Comment on lines +402 to +405
'companionUrl' in opts ||
'companionHeaders' in opts ||
'companionCookiesRule' in opts ||
'companionKeysParams' in opts
Copy link
Member

Choose a reason for hiding this comment

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

forgot about these, sounds quite awkward if you want to send headers along to your own backend and you have to write endpoint and companionHeaders. Do we have to rename all then?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other awkward thing would be if one could not share the options with remote sources plugin, I’m not sure how to approach this.

Copy link
Member

Choose a reason for hiding this comment

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

But this is not a remote source plugin, so there is no problem of inconsistency as this is an uploader? I think we can safely change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't you think there are users that use remote sources with AWS plugin?

Copy link
Member

@Murderlon Murderlon May 16, 2024

Choose a reason for hiding this comment

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

It's possible, but then they have to just endpoint, headers, cookiesRule here. I don't think it matters. Note that people have to do this with xhr and tus too, so if anything it just aligns it more.

companionUrl: string
type AWSS3WithCompanion = (
| {
/** @deprecated use `endpoint` instead */
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

On the next major

@Murderlon
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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