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
aws-s3-multipart: make headers
part indexed too in prepareUploadParts
#3895
Conversation
Thoughts @kevin-west-10x @daghendrik? |
@ifedapoolarewaju if you have a moment too, please |
If you are making breaking changes anyway, it might be easier to do something like this:
This way, you aren't forced into batching requests, you can opt in by overriding the right function. Additionally, instead of having
So that each part number has it's own url and set of headers. |
We could do that, but it would be creating unnecessary overhead again with multiple async operations we have to wait for. The likely flow is, to not expose secrets or to save bundle size by not adding a hashing library, is that people call their backend to generate the urls and then send it back to Uppy. This means a lot of roundtrips, instead of a single roundtrip with a batch of parts. Additionally, it feels a bit weird to go in circles and go back to the previous API 😅
I deliberately didn't do this. Even though we can make breaking changes in Uppy easily now, for Companion we have a much stricter policy when it comes to breaking changes. This is because all versions of Uppy request to a single Companion version in Transloadit. For the previous breaking change we kept the old endpoint around for backwards compatibility and let Uppy v2 use the new This time around though we aren't introducing a new endpoint and we can't make breaking changes to the batch endpoint because then it would only work for Uppy v3 clients, while many customers would still be on v2. Companion only uses the
This means we can make a breaking change in |
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.
nice!
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Fair enough, this seems like a perfectly fine solution given the requirements and it should unblock me. If most people make these calls batched to the backend it makes sense to optimize for that and make sure it works ok for the less common cases as well. |
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.
Hi 👋🏽
I never worked much on the s3 side of things, so I may not have as much insight here.
Hence, I have only shared some comments based on a more high-level review.
Other than the comments shared, all else LGTM 👍🏽
@@ -109,13 +109,12 @@ A function that generates a batch of signed URLs for the specified part numbers. | |||
|
|||
* `uploadId` - The UploadID of this Multipart upload. | |||
* `key` - The object key in the S3 bucket. | |||
* `partNumbers` - An array of indices of this part in the file (`PartNumber` in S3 terminology). Note that part numbers are _not_ zero-based. | |||
* `chunks` - A Javascript object with the part numbers as keys and the Blob data for each part as the value. | |||
* `parts` - An array of objects with the part number and chunk (`Array<{ number: number, chunk: blob }>`). `number` can’t be zero. |
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.
has there been consideration for using partNumber
in place of number
? So it would read as Array<{ partNumber: number, chunk: blob }>
instead.
It might just be me, but reading { number: number }
kind of trips one up a little bit at first glance.
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.
Personally, I prefer making keys intention revealing with the context of the parent object. part.number
already implies it's the part's number, wheres part.partNumber
sounds like a pleonasm to me.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
| Package | Version | Package | Version | | ---------------------- | ------------ | ---------------------- | ------------ | | @uppy/aws-s3 | 3.0.0-beta.2 | @uppy/react | 3.0.0-beta.2 | | @uppy/aws-s3-multipart | 3.0.0-beta.2 | @uppy/remote-sources | 1.0.0-beta.2 | | @uppy/companion | 4.0.0-beta.2 | @uppy/store-redux | 3.0.0-beta.2 | | @uppy/compressor | 1.0.0-beta.2 | @uppy/transloadit | 3.0.0-beta.3 | | @uppy/core | 3.0.0-beta.2 | @uppy/webcam | 3.0.0-beta.2 | | @uppy/dashboard | 3.0.0-beta.2 | @uppy/xhr-upload | 3.0.0-beta.2 | | @uppy/image-editor | 2.0.0-beta.2 | @uppy/robodog | 3.0.0-beta.3 | | @uppy/locales | 3.0.0-beta.3 | uppy | 3.0.0-beta.3 | - @uppy/react: Fix exports in propTypes.js to fix website build (Murderlon) - @uppy/dashboard,@uppy/webcam: Add support for `mobileNativeCamera` option to Webcam and Dashboard (Artur Paikin / #3844) - @uppy/aws-s3-multipart: make `headers` part indexed too in `prepareUploadParts` (Merlijn Vos / #3895) - @uppy/aws-s3,@uppy/core,@uppy/dashboard,@uppy/store-redux,@uppy/xhr-upload: upgrade `nanoid` to v4 (Antoine du Hamel / #3904) - @uppy/companion: update minimal supported Node.js version in the docs (Antoine du Hamel / #3902) - @uppy/companion: upgrade `redis` to version 4.x (Antoine du Hamel / #3589) - @uppy/companion: remove unnecessary ts-ignores (Mikael Finstad / #3900) - meta: use `node:` protocol when using Node.js built-in core modules (Antoine du Hamel / #3871) - meta: upgrade to Vite v3 (Antoine du Hamel / #3882) - @uppy/companion: remove `COMPANION_S3_GETKEY_SAFE_BEHAVIOR` env variable (Antoine du Hamel / #3869) - meta: fix release script for major beta versions (Antoine du Hamel)
* 3.x: (32 commits) @uppy/screen-capture: fix TODOs (#3930) @uppy/status-bar: rename internal modules (#3929) @uppy/transloadit: remove static properties in favor of exports (#3927) @uppy/informer: simplify `render` method (#3931) @uppy/url: remove private methods from public API (#3934) @uppy/dashboard: change `copyToClipboard` signature (#3933) @uppy/drop-target: remove `isFileTransfer` from the public API (#3932) meta: improve beta release script Release: uppy@3.0.0-beta.3 (#3918) Release: uppy@2.13.1 (#3916) Fix exports in propTypes.js to fix website build @uppy/compressor: fix upload causing meta name to reset (#3890) @uppy/transloadit: cancel assemblies when all its files have been removed (#3893) Add retries for flaky e2e test (#3915) Fix `uppy.close()` crashes when remote-sources or image-editor is installed (#3914) webcam: Add support for mobileNativeCamera option to Webcam and Dashboard (#3844) aws-s3-multipart: make `headers` part indexed too in `prepareUploadParts` (#3895) Add missing type for retry-all event (#3901) Companion app type (#3899) e2e: upgrade to Cypress 10 (#3896) ...
| Package | Version | Package | Version | | ---------------------- | ------------ | ---------------------- | ------------ | | @uppy/aws-s3 | 3.0.0-beta.2 | @uppy/react | 3.0.0-beta.2 | | @uppy/aws-s3-multipart | 3.0.0-beta.2 | @uppy/remote-sources | 1.0.0-beta.2 | | @uppy/companion | 4.0.0-beta.2 | @uppy/store-redux | 3.0.0-beta.2 | | @uppy/compressor | 1.0.0-beta.2 | @uppy/transloadit | 3.0.0-beta.3 | | @uppy/core | 3.0.0-beta.2 | @uppy/webcam | 3.0.0-beta.2 | | @uppy/dashboard | 3.0.0-beta.2 | @uppy/xhr-upload | 3.0.0-beta.2 | | @uppy/image-editor | 2.0.0-beta.2 | @uppy/robodog | 3.0.0-beta.3 | | @uppy/locales | 3.0.0-beta.3 | uppy | 3.0.0-beta.3 | - @uppy/react: Fix exports in propTypes.js to fix website build (Murderlon) - @uppy/dashboard,@uppy/webcam: Add support for `mobileNativeCamera` option to Webcam and Dashboard (Artur Paikin / transloadit#3844) - @uppy/aws-s3-multipart: make `headers` part indexed too in `prepareUploadParts` (Merlijn Vos / transloadit#3895) - @uppy/aws-s3,@uppy/core,@uppy/dashboard,@uppy/store-redux,@uppy/xhr-upload: upgrade `nanoid` to v4 (Antoine du Hamel / transloadit#3904) - @uppy/companion: update minimal supported Node.js version in the docs (Antoine du Hamel / transloadit#3902) - @uppy/companion: upgrade `redis` to version 4.x (Antoine du Hamel / transloadit#3589) - @uppy/companion: remove unnecessary ts-ignores (Mikael Finstad / transloadit#3900) - meta: use `node:` protocol when using Node.js built-in core modules (Antoine du Hamel / transloadit#3871) - meta: upgrade to Vite v3 (Antoine du Hamel / transloadit#3882) - @uppy/companion: remove `COMPANION_S3_GETKEY_SAFE_BEHAVIOR` env variable (Antoine du Hamel / transloadit#3869) - meta: fix release script for major beta versions (Antoine du Hamel)
Closes #3881
Unfortunately, my proposal didn't work because 1) the default Companion implementation needs to match the custom provided one and 2) it was naive of me to think you always want to do it on the client. If you want to get your presigned URLs from your own server, you can't send back
new Request
as JSON. It's then superfluous to come up with your own response, to then manually turn those intoRequest
's on the client (might as well use the response directly).New approach:
headers
part indexed too, likepresignedUrls
. It's not pretty but it works.Optional todos:
listParts
andcompleteMultipartUpload
also change?