Support for additional S3 providers #71
Conversation
src/app/api/s3-upload/route.ts
Outdated
| accessKeyId: process.env.S3_UPLOAD_KEY, | ||
| secretAccessKey: process.env.S3_UPLOAD_SECRET, | ||
| bucket: process.env.S3_UPLOAD_BUCKET, | ||
| region: process.env.S3_UPLOAD_REGION, |
There was a problem hiding this comment.
these are superfluous i think, only endpoint and forcePathStyle are needed if the rest are the default env vars used
There was a problem hiding this comment.
Thank you! I removed them as they were not needed indeed.
| @@ -29,7 +29,7 @@ type Props = { | |||
|
|
|||
| export function ExpenseDocumentsInput({ documents, updateDocuments }: Props) { | |||
| const [pending, setPending] = useState(false) | |||
| const { FileInput, openFileDialog, uploadToS3 } = useS3Upload() | |||
| const { FileInput, openFileDialog, uploadToS3 } = usePresignedUpload() // use presigned uploads to addtionally support providers other than AWS | |||
There was a problem hiding this comment.
Not sure about this (since I'm no expert on js/ts), but you could continue to use the useS3Upload() function if !!process.env.S3_UPLOAD_ENDPOINT, since it's recommended for various reasons by the next-s3-upload lib.
So import both, and set the variables depending on the env var.
If this doesn't work for whatever client/server magic reasons that always happens with next, you may also ignore this comment :)
There was a problem hiding this comment.
Since this is on the client side, the environment variable would need to be exposed using the NEXT_PUBLIC_ prefix and defined at build time. This leads to two options:
- use
useS3Uploadby default since it is recommended and add a slightly redundant environment variable (e.g.NEXT_PUBLIC_ENABLE_CUSTOM_S3_ENDPOINT) that indicates whetherusePresignedUploadshould be used instead. - use
usePresignedUploadfor any provider since the only documented limitation is that AWS S3 has a file limit of 1GB on presigned uploads
I went with the latter option for a bit less complexity and because I assumed that there is no current use case for uploads larger than 1GB, but I am open to changing the approach.
There was a problem hiding this comment.
Yeah, that's what I meant about the client/server magic, so then I agree that this is the better solution, but @scastiel has the last word on the PR anyhow 😄
There was a problem hiding this comment.
Agree to use usePresignedUpload everytime if it doesn’t cause any issue with S3. Plus it’s tricky to use a hook conditionally with React 😉
src/app/api/s3-upload/route.ts
Outdated
| endpoint: process.env.S3_UPLOAD_ENDPOINT, | ||
| // forcing path style is only necessary for providers other than AWS | ||
| forcePathStyle: !!process.env.S3_UPLOAD_ENDPOINT, |
There was a problem hiding this comment.
Prefer using env from @/lib/env when on the server, as you get type safe values 😉
endpoint: env.S3_UPLOAD_ENDPOINT,
// forcing path style is only necessary for providers other than AWS
forcePathStyle: !!env.S3_UPLOAD_ENDPOINT,There was a problem hiding this comment.
Thank you! I am now using the typed environment variables. I also resolved the merge conflicts introduced by the recent changes on main.
| @@ -29,7 +29,7 @@ type Props = { | |||
|
|
|||
| export function ExpenseDocumentsInput({ documents, updateDocuments }: Props) { | |||
| const [pending, setPending] = useState(false) | |||
| const { FileInput, openFileDialog, uploadToS3 } = useS3Upload() | |||
| const { FileInput, openFileDialog, uploadToS3 } = usePresignedUpload() // use presigned uploads to addtionally support providers other than AWS | |||
There was a problem hiding this comment.
Agree to use usePresignedUpload everytime if it doesn’t cause any issue with S3. Plus it’s tricky to use a hook conditionally with React 😉
|
I did try this out towards my own hosted version of Minio (behind a reverse proxy) and it works like a charm 👌 EDIT: Also tested that it works when compiled, with the docker image |
|
Just tested that everything still works with the S3 bucket 👍 |
Adding support for additional S3 providers to avoid vendor lock-in and for a better self-hosting experience. See #65
I successfully tested integration with a local instance of Minio. I have not tested these changes with a standard AWS S3 setup.