aws-s3-multipart: make headers part indexed too in prepareUploadParts

[Murderlon]

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 into Request's on the client (might as well use the response directly).

New approach:

• Make headers part indexed too, like presignedUrls. It's not pretty but it works.
• Add a test for it.
• Update docs.
• Update types.

Optional todos:

• Should listParts and completeMultipartUpload also change?
• Any other breaking changes in aws-s3-multipart wanted?

[Murderlon]

Thoughts @kevin-west-10x @daghendrik?

[arturi]

@ifedapoolarewaju if you have a moment too, please

[kevin-west-10x]

If you are making breaking changes anyway, it might be easier to do something like this:

• Add back prepareUploadPart as a function you can provide.
• Default implementation of prepareUploadParts simply calls prepareUploadPart with each part.
• You can choose to override prepareUploadParts with your own batch implementation if you want.

This way, you aren't forced into batching requests, you can opt in by overriding the right function.

Additionally, instead of having presignedUrls and headers, I think it would be better to expect something more like

{
  [partNumber]: {
    presignedUrl,
    headers
  }
}

So that each part number has it's own url and set of headers.

[Murderlon]

Default implementation of prepareUploadParts simply calls prepareUploadPart with each part.

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 😅

Additionally, instead of having presignedUrls and headers, I think it would be better to expect something more like

{
  [partNumber]: {
    presignedUrl,
    headers
  }
}

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 */batch/* endpoint.

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 presignedUrls:

uppy/packages/@uppy/companion/src/server/controllers/s3.js

Line 261 in a81b948

res.json({ presignedUrls })

This means we can make a breaking change in headers without effecting other versions.

[mifi]

nice!

[kevin-west-10x]

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.

[ifedapoolarewaju]

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 👍🏽

[ifedapoolarewaju]

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.

[Murderlon]

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.