feat(storage): Implement S3 Storage Adapter to be used with AWS S3 or compatible service like Fly's Tigris

[dthyresson]

This PR adds a S3Storage adapter to Redwood Storage to be used with AWS S3 or compatible service like Fly's Tigris.

• S3Storage - the adapter that can read, save, remove and sign files to/from/in S3 storage
• S3UrlSigner - signed used by the Prisma extension to sign the file with the S3 sdk

Note: The withSignedUrl is now async just like the withDataUri is since for S3 the get signed url function in async. Therefore, some change are made to Prisma extension that if anyone is using, will need to be made async.

S3Storage also lets you define cache control headers.

When using S3Storage one does not need to use or have the signedUrl Redwood function to validate the url signation since this is all done by S3/Tigris. This save compute/requests on the app and ensures you get all the benefits of the S3 CDN to cache.

[Josh-Walker-GM]

I'm happy to review this but it'll be at least Monday (16th) before I do. We'll probably want this it it's own adapter package, like for mail handlers, since it brings in specific dependencies but I'm happy to do that sort of chore work since you've done the more important implementation part already here.

[Tobbe]

Does having async + await here make a difference? I thing you'll be returning an array of promises either way, but here you're creating an extra layer of promise wrapping (which the JIT compiler might be smart enough to remove, idk)

[dthyresson]

I don't know enough about promises to say one way or the other. I know that I need the withSignedUrl to be async so that the s3 sdk can call:

  async sign(fileLocation: string, expiresIn = 3600): Promise<string> {
    const command = new GetObjectCommand({
      Bucket: this.bucket,
      Key: fileLocation,
    })

    return await getSignedUrl(this.s3Client, command, { expiresIn })
  }

Without it didn't work.

[Tobbe]

Yeah, I totally see why you had to make it async. No question there 🙂 Just don't think this need the extra async/await. But please don't just take my word for it. Try it out in a real project to make sure.

[dthyresson]

I did. Without the await in my describe pic and remove background job when I asked for the signedUrl it was undefined. When I made it async I got the url. Maybe within the service you do r need it but in a job I did so I could get the url and pass it along to fal as an image url reference.

[dthyresson]

Also the dataUri compute helper was always async so I just made both compute helpers work the same.

[Tobbe]

Yes, that all makes perfect sense. When you need to use the value for further computations/logic you need to await it first. But in the example you're just straight away returning it. That's why I don't think you need to await it. Just just passing the promise along.

This is the current example:

return filesViaRelation.map(async (file) => await file.withSignedUrl())

I'm just saying that in that particular case I think you could just have done this

return filesViaRelation.map((file) => file.withSignedUrl())

Because withSignedUrl() returns a promise and that promise is then used as the return value of the map callback.

If you had to use file inside the callback after calling withSignedUrl() you would have had to await it first. Like this:

return filesViaRelation.map(async (file) => {
  const fileWithSignedUrl = await file.withSignedUrl()
  console.log('fileWithSignedUrl', fileWithSignedUrl)

  return fileWithSignedUrl
})

So yes, withSignedUrl() always has to be async. But whether you explicitly need to await it or not depends on how you use it.

I took an extra look inside S3Storage.ts and now see that I have similar feedback there. I think we should directly return the promise in remove and sign instead of first awaiting it and then returning.

But that requires some changes to BaseStorageAdapter. So maybe hold off on these changes until after we've discussed our current implementation on Tuesday

[Tobbe]

I don't think I've seen this before where you both extend and implement the same base class. What does that actually mean? What does it do?

[dthyresson]

Not sure but again I followed the pattern established that the File and Memory adapters use in current main

redwood/packages/storage/src/adapters/FileSystemStorage/FileSystemStorage.ts

Line 12 in e384c39

export class FileSystemStorage

[dthyresson]

Now that I think about it the base adapter defines the common read etc but some also implement a remove and sign. So perhaps implements makes sure the common methods are implemented and then extend says we have some new custom methods like sign or remove but still consider the class to be like the base adapter? Thus the extend?

[dthyresson]

I decided to mark this as a draft with some upcoming Storage refactoring. It makes more sense to add a S3 adapter after that happens.

[dthyresson]

Closing as we'll have a new interface for adapters soon.