Skip to content
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

Adding Dynamic bucket & region params to config #169

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DhenPadilla
Copy link

Context

We're using Next-s3-upload to upload to a single bucket currently.
This sufficed for a single use case up until now as we have a strong security requirement to keep the current bucket that we're uploading to be completely READ-restricted to public access (i.e private to anything external to aws). This cannot change.

We want to continue using next-s3-upload for our new use case:
Uploading files to a new bucket which would have public read access.
Currently, next-s3-upload does not allow for dynamically allocating a bucket name and only saves to the S3_UPLOAD_BUCKET env var. This is fine for our initial use case, but definitely not moving forward as we may need more than 2 buckets but especially more than 1.

Note: Although we're using 1 region, I imagine we (or other users) may use multiple regions now in the future.

I imagine this would help other users of the package, so hopefully this is a solid PR!

@vercel
Copy link

vercel bot commented Oct 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-s3-upload ❌ Failed (Inspect) Oct 30, 2023 2:45am

@ryanto
Copy link
Owner

ryanto commented Oct 28, 2023

Cool, thanks for the PR!

I'd like to understand this a little better... can you show me how'd you use this with code? Do you have if statements inside your bucket function?

@DhenPadilla
Copy link
Author

DhenPadilla commented Oct 28, 2023

@ryanto

No worries!
Of course,
Here's a snippet of how I intend to use this:

async bucket(req) {
    const { uploadType } = req.body as { uploadType: S3UploadType }
    const bucketName =
        uploadType === S3UploadType.image
            ? process.env.S3_PUBLIC_BUCKET
            : process.env.S3_PRIVATE_BUCKET
            
    // May extend to more options in the future 
    return bucketName
}

Omitting S3UploadType type from the code here as it would be 1st-party

@DhenPadilla DhenPadilla reopened this Oct 28, 2023
@ryanto
Copy link
Owner

ryanto commented Oct 29, 2023

Ok cool , makes sense! I'll probably need a few days before I can get this merged and published.

One thing that would help speed things up is if you could add docs to this page: https://next-s3-upload.codingvalue.com/bucket-config

@DhenPadilla
Copy link
Author

Ok cool , makes sense! I'll probably need a few days before I can get this merged and published.

One thing that would help speed things up is if you could add docs to this page: https://next-s3-upload.codingvalue.com/bucket-config

Yep!
Done!

@DhenPadilla
Copy link
Author

@ryanto
Would be amazing to have this in soon as I do need this build for this week if possible! :')

@DhenPadilla
Copy link
Author

Bumping this when possible!

@anthonykrivonos
Copy link

Bumping this too, my team would benefit from dynamic bucket functionality as we're storing data in both a public and private bucket.

Great work here @DhenPadilla. Sorry to spam but a merge would be great @ryanto 🙏

@anthonykrivonos
Copy link

@DhenPadilla Thanks for your blessed contribution. We couldn't wait any longer so we created a fork with your changes.

yarn add @toma.so/next-s3-upload

@douglasqian
Copy link

Hi @ryanto! Is there anything that's blocking merging this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants