@uppy/aws-s3,@uppy/aws-s3-multipart; fixes #4575 updating types to match web docs #4576

bdirito · 2023-07-13T21:07:22Z

Given the types were different between the typescript types & the web documentation I took the approach of assuming the web documents were correct. This may not actually be the case but I didn't have the time to dig into the underlying code to find out.

There are a couple of particular other changes that were questionable. I've tried to point those out in a review.

…tation

packages/@uppy/aws-s3-multipart/types/index.d.ts

bdirito · 2023-07-13T21:09:44Z

packages/@uppy/aws-s3-multipart/types/index.d.ts

@@ -22,7 +22,8 @@ export interface AwsS3STSResponse {
  region: string
 }

-export interface AwsS3MultipartOptions extends PluginOptions {


PluginOptions contains only an id and a replaceTargetContent fields. Given that replaceTargetContent is not used / documented I removed the inheritance.

I think we want to keep the inheritence, even if we don't use the parents definitions, it's still useful to match the type PluginOptions.

Ok; I dug into the code a bit here. It looks like the option replaceTargetContent is used in cases where we are using a UIPlugin uppy/packages/@uppy/core/src/UIPlugin.js:90. UIPlugin extends BasePlugin. Neither AwsS3 or AwsS3Multipart is a UIPlugin. Their inheritance is AwsS3[Multipart] extends UploaderPlugin extends BasePlugin.

I disagree strongly with having types that dont actually match reality. However I think by changing the types to actually reflect the real inheritance we can have inheritance and have proper types.

Thus i propose:
BasePluginOptions UIPluginOptions UploaderPluginOptions
replaceTargetContents becomes a member of UIPluginOptions
id becomes a member of BasePluginOptions (though it doesn't appear to be an actual inherited attribute but rather implemented in each Plugin in order to have it default to the plugin name - see uppy/packages/@uppy/aws-s3/src/index.js:122 for example)

@Murderlon is considering removing UploaderPlugin in #4573, so maybe we shouldn't include it in our plans

Ok - i pushed new types such that classes that extend UIPlugin take UIPluginOptions; classes that extend BasePlugin take PluginOptions. Other plugins have been updated with this type information though I did not check for type correctness beyond updating plugins that cited inheriting from UIPlugin to have their options inherit from UIPluginOptions

bdirito · 2023-07-13T21:11:24Z

packages/@uppy/aws-s3-multipart/types/index.d.ts

    signPart?: (
      file: UppyFile,
-      opts: { uploadId: string; key: string; partNumber: number; body: Blob, signal: AbortSignal }
+      opts: { uploadId: string; key: string; partNumber: number; body: Blob; signal: AbortSignal }


I tried to match the style as it was done; however this appears to be an inconsistency so I did change it here.

packages/@uppy/aws-s3-multipart/types/index.d.ts

bdirito · 2023-07-13T21:15:30Z

packages/@uppy/aws-s3/types/index.d.ts

@@ -10,8 +9,20 @@ export interface AwsS3UploadParameters {
    headers?: { [type: string]: string }
 }

-export interface AwsS3Options extends AwsS3MultipartOptions {
-    /** @deprecated future versions of this plugin will use the Expires value from the backend */
+export interface AwsS3Options {


There I removed the inheritance from AwsS3MultipartOptions. Its possible that they should both inherit from a BaseS3Options but I did not do that here.

bdirito · 2023-07-13T21:24:40Z

packages/@uppy/aws-s3/types/index.d.ts

+    getUploadParameters?: (file: UppyFile) => MaybePromise<{
+      method: "PUT" | "POST"
+      url: string
+      fields: Record<string, string> | null


fields was a difficult one. On the one hand the docs say that the promise should fulfills with an object, with keys { method, url, fields, headers }. To have the key to me means it should not be allowed to be undefined.

However it also says that For presigned PUT uploads, this should be left empty.

I decided that empty but defined meant null was an option.

The docs say the other option was to return an object with form fields. Record<string, string> is almost certainly too strict; I would expect it to actually support something like Record<string, string | Array<string> | number | Array<number>> or even more. But I wanted to default into something as strict as possible over just using something like Object;

I think a better type would be

{ method: 'POST' url: string fields: Record<string,string> } | { method: 'PUT' url: string }

bdirito · 2023-07-13T21:25:14Z

packages/@uppy/aws-s3/types/index.d.ts

+      method: "PUT" | "POST"
+      url: string
+      fields: Record<string, string> | null
+      headers: Record<string, string>


In the same way as above Record<string, string> is probably too strict.

packages/@uppy/aws-s3/types/index.d.ts

bdirito · 2023-07-13T21:40:11Z

packages/@uppy/aws-s3/types/index.test-d.ts

 import AwsS3 from '..'

 {
  const uppy = new Uppy()
  uppy.use(AwsS3, {
    getUploadParameters (file) {
      expectType<UppyFile>(file)
-      return { url: '' }
+      return { method: 'POST', url: '', fields: null, headers: {} }


Not sure what we want for this. These seemed sane test values.

bdirito · 2023-07-13T22:07:26Z

packages/@uppy/aws-s3-multipart/types/index.d.ts

-      partData: { uploadId: string; key: string; parts: [{ number: number, chunk: Blob }], signal: AbortSignal }
-    ) => MaybePromise<{ presignedUrls: { [k: number]: string }, headers?: { [k: string]: string } }>
+      partData: { uploadId: string; key: string; parts: [{ number: number, chunk: Blob }] }
+    ) => MaybePromise<{ presignedUrls: { [k: number]: string }, headers?: Record<number, Record<string, string>> }>


The webpage documentation (and examples) do not match the types. In particular it looks like the types are missing an object level.

From the docs as an example return value:

{ "presignedUrls": { "1": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...", "2": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...", "3": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&..." }, "headers": { "1": { "Content-MD5": "foo" }, "2": { "Content-MD5": "bar" }, "3": { "Content-MD5": "baz" } } }

Why do we have k: number for presignedUrls and Record for headers?

I don't know if there is any sort of Uppy standard as to the preferred approach. Record is definitely used throughout the codebase as well as the {[k: type]: type} syntax. For example the core types use the Record style as in packages/@uppy/core/types/index.d.ts. I think I would suspect that the core style to be more likely to be the preferred approach.

Either way styles should probably not be mixed across a single file, or worse a single function type declaration. Thus I have updated the presignedUrl here to use Record as well

…lugin classes use UIPluginOptions; BasePlugin uses PluginOptions

…ses a 'PUT' method it must not have 'fields'

…run some prebuild step to establish imports

Murderlon

Thanks a lot for diving deep into this 👍

Murderlon · 2023-07-18T09:19:14Z

packages/@uppy/aws-s3-multipart/types/index.d.ts

-      partData: { uploadId: string; key: string; parts: [{ number: number, chunk: Blob }], signal: AbortSignal }
-    ) => MaybePromise<{ presignedUrls: { [k: number]: string }, headers?: { [k: string]: string } }>
+      partData: { uploadId: string; key: string; parts: [{ number: number, chunk: Blob }] }
+    ) => MaybePromise<{ presignedUrls: { [k: number]: string }, headers?: Record<number, Record<string, string>> }>


Why do we have k: number for presignedUrls and Record for headers?

…elper type

Murderlon

One Q, otherwise LGTM

packages/@uppy/aws-s3/types/index.d.ts

packages/@uppy/aws-s3-multipart/types/index.d.ts

aduh95

Thanks ❤️

| Package | Version | Package | Version | | ------------------------- | ------- | ------------------------- | ------- | | @uppy/audio | 1.1.2 | @uppy/locales | 3.3.0 | | @uppy/aws-s3 | 3.2.2 | @uppy/onedrive | 3.1.3 | | @uppy/aws-s3-multipart | 3.5.3 | @uppy/progress-bar | 3.0.3 | | @uppy/box | 2.1.3 | @uppy/provider-views | 3.5.0 | | @uppy/companion | 4.8.0 | @uppy/redux-dev-tools | 3.0.3 | | @uppy/companion-client | 3.3.0 | @uppy/screen-capture | 3.1.2 | | @uppy/core | 3.4.0 | @uppy/status-bar | 3.2.4 | | @uppy/dashboard | 3.5.1 | @uppy/thumbnail-generator | 3.0.4 | | @uppy/drag-drop | 3.0.3 | @uppy/transloadit | 3.2.1 | | @uppy/dropbox | 3.1.3 | @uppy/tus | 3.1.3 | | @uppy/facebook | 3.1.2 | @uppy/unsplash | 3.2.2 | | @uppy/file-input | 3.0.3 | @uppy/url | 3.3.3 | | @uppy/google-drive | 3.2.1 | @uppy/webcam | 3.3.2 | | @uppy/image-editor | 2.1.3 | @uppy/xhr-upload | 3.3.2 | | @uppy/informer | 3.0.3 | @uppy/zoom | 2.1.2 | | @uppy/instagram | 3.1.2 | uppy | 3.14.0 | - meta: Readme improvements (Artur Paikin / #4622) - @uppy/companion: Fix typos and add env vars to .env.example (Dominik Schmidt / #4624) - @uppy/aws-s3-multipart: pass the `uploadURL` back to the caller (Antoine du Hamel / #4614) - meta: update to node-18.17.0-alpine, (odselsevier / #4617) - @uppy/aws-s3,@uppy/aws-s3-multipart: update types (Antoine du Hamel / #4611) - @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/companion,@uppy/transloadit,@uppy/xhr-upload: use uppercase HTTP method names (Antoine du Hamel / #4612) - meta: e2e: fix race condition in transloadit test (Antoine du Hamel / #4616) - @uppy/aws-s3,@uppy/aws-s3-multipart: update types (bdirito / #4576) - @uppy/core: allow duplicate files with onBeforeFileAdded (Merlijn Vos / #4594) - @uppy/companion: make CSRF protection helpers available to providers (Dominik Schmidt / #4554) - @uppy/companion: fix Redis key default TTL (Subha Sarkar / #4607) - @uppy/companion: Fix Uploader.js metadata normalisation (Subha Sarkar / #4608) - @uppy/companion-client,@uppy/provider-views: make authentication optional (Dominik Schmidt / #4556) - @uppy/provider-views: fix ProviderView error on empty plugin.icon (Dominik Schmidt / #4553) - @uppy/aws-s3,@uppy/tus,@uppy/xhr-upload: Invoke headers function for remote uploads (Dominik Schmidt / #4596) - @uppy/companion: Unify redis initialization (Dominik Schmidt / #4597) - meta: lock node-js version on ci (Mikael Finstad / #4606) - @uppy/companion: allow dynamic S3 bucket (rmoura-92 / #4579) - @uppy/status-bar: e2e: add test for retrying and pausing uploads (Antoine du Hamel / #3599) - meta: e2e: remove too short timeout (Antoine du Hamel / #4602)

* main: (28 commits) Release: uppy@3.14.1 (#4644) @uppy/aws-s3-multipart: fix types when using deprecated option (#4634) @uppy/companion: harden lint rules (#4641) allow empty objects for `fields` types (#4631) meta: upgrade Node.js docker version (#4630) Release: uppy@3.14.0 (#4626) Readme improvements (#4622) Fix typos and add env vars to .env.example (#4624) @uppy/aws-s3-multipart: pass the `uploadURL` back to the caller (#4614) update to node-18.17.0-alpine, (#4617) @uppy/aws-s3,@uppy/aws-s3-multipart: update types (#4611) use uppercase HTTP method names (#4612) e2e: fix race condition in transloadit test (#4616) @uppy/aws-s3,@uppy/aws-s3-multipart: update types (#4576) @uppy/core: allow duplicate files with onBeforeFileAdded (#4594) @uppy/companion: make CSRF protection helpers available to providers (#4554) @uppy/companion: fix Redis key default TTL (#4607) @uppy/companion: Fix Uploader.js metadata normalisation (#4608) @uppy/companion-client,@uppy/provider-views: make authentication optional (#4556) @uppy/provider-views: fix ProviderView error on empty plugin.icon (#4553) ...

updating types of s3options & s3multipartOptions to match web documen…

aad42c1

…tation

github-actions bot added the pending end-to-end tests label Jul 13, 2023

bdirito commented Jul 13, 2023

View reviewed changes

bdirito added 2 commits July 13, 2023 15:49

using Locale type and fixing lint errors

f4de7c3

missed the web site docs on prepareUploadParts - adding that back in

f264ab5

bdirito commented Jul 13, 2023

View reviewed changes

bdirito changed the title ~~fixes #4575 updating types of s3options & s3multipartOptions to match web docs~~ @uppy/aws-s3; @uppy/aws-s3-multipart; fixes #4575 updating types to match web docs Jul 13, 2023

bdirito changed the title ~~@uppy/aws-s3; @uppy/aws-s3-multipart; fixes #4575 updating types to match web docs~~ @uppy/aws-s3,@uppy/aws-s3-multipart; fixes #4575 updating types to match web docs Jul 13, 2023

Murderlon requested a review from aduh95 July 17, 2023 08:48

bdirito added 4 commits July 17, 2023 14:20

refactoring PluginOptions to PluginOptions & UIPluginOptions. All UIP…

70ae994

…lugin classes use UIPluginOptions; BasePlugin uses PluginOptions

updates the type information to reflect that if getUploadParameters u…

d1b0c69

…ses a 'PUT' method it must not have 'fields'

fixes aws-s3 type tests; skips precommit hooks on the hope I need to …

ba59c2e

…run some prebuild step to establish imports

fixing google-drive type checking

afe6572

Murderlon reviewed Jul 18, 2023

View reviewed changes

converting to be consistant on prepareUploadParts in using Record<> h…

cd8a584

…elper type

Murderlon approved these changes Jul 24, 2023

View reviewed changes

packages/@uppy/aws-s3/types/index.d.ts Show resolved Hide resolved

bdirito and others added 2 commits July 25, 2023 11:38

asserts that 'fields' is invalid for a 'PUT' request

018b34c

Merge branch 'main' into s3types

ec2b605

aduh95 reviewed Aug 7, 2023

View reviewed changes

packages/@uppy/aws-s3-multipart/types/index.d.ts Outdated Show resolved Hide resolved

aduh95 reviewed Aug 7, 2023

View reviewed changes

packages/@uppy/aws-s3-multipart/types/index.d.ts Outdated Show resolved Hide resolved

aduh95 reviewed Aug 7, 2023

View reviewed changes

packages/@uppy/aws-s3-multipart/types/index.d.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

f63d828

aduh95 approved these changes Aug 7, 2023

View reviewed changes

aduh95 linked an issue Aug 7, 2023 that may be closed by this pull request

@uppy/AwsS3 and @uppy/AwsS3Multipart options type information does not match the documentation #4575

Closed

2 tasks

aduh95 merged commit f3ce884 into transloadit:main Aug 7, 2023

github-actions bot mentioned this pull request Aug 15, 2023

Release: uppy@3.14.0 #4626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@uppy/aws-s3,@uppy/aws-s3-multipart; fixes #4575 updating types to match web docs #4576

@uppy/aws-s3,@uppy/aws-s3-multipart; fixes #4575 updating types to match web docs #4576

bdirito commented Jul 13, 2023

bdirito Jul 13, 2023

aduh95 Jul 17, 2023

bdirito Jul 17, 2023 •

edited

Loading

aduh95 Jul 17, 2023

bdirito Jul 17, 2023

This comment was marked as resolved.

This comment was marked as resolved.

bdirito Jul 13, 2023

bdirito Jul 13, 2023

bdirito Jul 13, 2023

aduh95 Jul 17, 2023

bdirito Jul 13, 2023

bdirito Jul 13, 2023

bdirito Jul 13, 2023

Murderlon Jul 18, 2023

bdirito Jul 20, 2023 •

edited

Loading

Murderlon left a comment

Murderlon Jul 18, 2023

Murderlon left a comment

aduh95 left a comment

@uppy/aws-s3,@uppy/aws-s3-multipart; fixes #4575 updating types to match web docs #4576

@uppy/aws-s3,@uppy/aws-s3-multipart; fixes #4575 updating types to match web docs #4576

Conversation

bdirito commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdirito Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

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

bdirito Jul 20, 2023 • edited Loading

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

Murderlon left a comment

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

bdirito Jul 17, 2023 •

edited

Loading

bdirito Jul 20, 2023 •

edited

Loading