-
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: add endpoint
option
#5173
base: 4.x
Are you sure you want to change the base?
Conversation
Diff output filesdiff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index 98fd0e6..617b713 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
Outdated
companionUrl: string | ||
type AWSS3WithCompanion = ( | ||
| { | ||
/** @deprecated use `endpoint` instead */ |
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.
I'm okay with doing a breaking change. If not now then when?
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.
On the next major
Still want to get this over the finish line? Things needed:
|
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.
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
docs/guides/migration-guides.md
Outdated
@@ -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`) |
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.
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
Outdated
@@ -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) => { |
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.
I wonder if it would be worth renaming the Companion endpoint /s3/params
is much less clear than /sign-s3
IMO
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.
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.
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.
I guess we could support both
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.
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.
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.
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.
@@ -385,10 +386,59 @@ export default class AwsS3Multipart< | |||
return this.#client | |||
} | |||
|
|||
#setClient(opts?: Partial<AwsS3MultipartOptions<M, B>>) { |
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.
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.
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 are breaking many other apis in 4.x, so why not break this too?
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.
This does not have any backward compatiblity, it's just emitting warnings if folks are using the old option names.
setOptions(newOptions: Partial<AwsS3MultipartOptions<M, B>>): void { | ||
this.#companionCommunicationQueue.setOptions(newOptions) | ||
super.setOptions(newOptions) | ||
this.#setCompanionHeaders() | ||
super.setOptions(newOptions as any) |
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.
something wrong here with the types?
super.setOptions(newOptions as any) | |
super.setOptions(newOptions as any) |
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.
Argument of type 'Partial<AwsS3MultipartOptions<M, B>>' is not assignable to parameter of type 'Partial<((DefinePluginOpts<AwsS3MultipartOptions<M, B>, "allowedMetaFields" | "limit" | "retryDelays" | "getTemporarySecurityCredentials" | "shouldUseMultipart"> & Pick<...> & Required<...>) & Partial<...>) & AWSS3MultipartWithoutCompanionMandatorySignPart<...> & AWSS3NonMultipartWithoutCompanionMandatory<...>>'.
Type 'Partial<_AwsS3MultipartOptions & AWSS3WithoutCompanion & AWSS3NonMultipartWithoutCompanionMandatory<M, B> & { ...; }>' is not assignable to type 'Partial<((DefinePluginOpts<AwsS3MultipartOptions<M, B>, "allowedMetaFields" | "limit" | "retryDelays" | "getTemporarySecurityCredentials" | "shouldUseMultipart"> & Pick<...> & Required<...>) & Partial<...>) & AWSS3MultipartWithoutCompanionMandatorySignPart<...> & AWSS3NonMultipartWithoutCompanionMandatory<...>>'.
// @ts-expect-error It's either a function or `false`, which should error. | ||
getTemporarySecurityCredentials(options) |
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.
// @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.
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.
This expression is not callable.
Type 'never' has no call signatures.
examples/aws-nodejs/index.js
Outdated
@@ -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) => { |
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.
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.
@@ -385,10 +386,59 @@ export default class AwsS3Multipart< | |||
return this.#client | |||
} | |||
|
|||
#setClient(opts?: Partial<AwsS3MultipartOptions<M, B>>) { |
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 are breaking many other apis in 4.x, so why not break this too?
I don't really get why I'm getting TS errors though