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

feat: Add aws-s3 driver (http) #361

Draft
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

becem-gharbi
Copy link

@becem-gharbi becem-gharbi commented Dec 29, 2023

πŸ”— Linked issue

#11

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

There is already an open PR (#96) for an implementation based on AWS sdk.

This PR adds an AWS s3 driver based on HTTP for a lightweight bundle size and edge runtime compatibility.

This is a WIP, I'm not sure if all driver methods can be implemented readme.

Progress

  • Implement main methods getItem getItemRaw setItem setItemRaw getKeys getMeta clear removeItem hasItem
  • Refactor & optimize
  • Add respective tests
  • Add respective docs

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@itpropro
Copy link
Member

itpropro commented Jan 2, 2024

Hi @becem-gharbi,
thanks for your PR! I would like to address a few points to help get the driver ready for merging:

  • Remove your playground folder from the PR
  • Add the required tests for your driver
  testDriver({
    driver: driver({
      DRIVER_OPTIONS
    }),
  });
  • Lazy load the connection for example with getAwsClient(), you can check some other drivers for similar implementations
  • Can we reevaluate on aws4fetch? It seems unmaintained and the preferred solution from Support s3 driverΒ #11 was direct HTTP, which should be possible with ofetch and uncrypto
  • Implement at least getItem, setItem and clear in addition to the existing methods for basic string based kv operations
  • Add respective docs for the driver

@becem-gharbi
Copy link
Author

Hi @itpropro, thanks for your help.
This solution is a direct HTTP client, aws4fetch is there to sign the request by generating headers for authorization. To be honest I'm not familiar with AWS signature. However the package is stable and works well as expected.

…ovided

For S3 compatible providers that do not implement ObjectsDelete or that have different API than the standard, the solution is to omit `accountId` option in order to perform multiple ObjectDelete instead.
Copy link

nuxt-studio bot commented Jan 6, 2024

βœ… Live Preview ready!

Name Edit Preview Latest Commit
unstorage Edit on Studio β†—οΈŽ View Live Preview 7b24a35

@becem-gharbi
Copy link
Author

Hi @itpropro, the driver is now ready to review. Regarding test, I'm not sure if using process.env is the way to pass driver options.

becem-gharbi added a commit to becem-gharbi/nuxt-s3 that referenced this pull request Jan 15, 2024
@becem-gharbi becem-gharbi marked this pull request as draft January 16, 2024 17:06
@becem-gharbi
Copy link
Author

Hi, I have recently converted this PR to a draft. The reason is because this driver is intended to be used with any S3 compatible service. The problem is that some differences exist in term of their implementations. These differences reflect in the driver usage. I have mentioned them in docs. I'm not sure about the current approach and I'm looking forward to your feedback.

@pi0
Copy link
Member

pi0 commented Mar 15, 2024

(@becem-gharbi sorry for delay on my side to give feedback, if you like, we can still keep it open or alternatively i can continue on this. just assuring you it was not intentionally left unattended)

@becem-gharbi becem-gharbi reopened this Mar 15, 2024
@productdevbook
Copy link

productdevbook commented Mar 16, 2024

@becem-gharbi Thank you, you've done a good job again. we need to make sure you support all s3 protocols. cloudflare r2, amazon s3 etc.

@pi0 can you prioritize this issue, because this is a need in many areas such as amazon and cloudflare r2. It is an expected feature to use nuxt-og-image together.

@pi0 pi0 added the driver label May 1, 2024
@pi0 pi0 changed the title feat: Add s3 http driver feat: Add aws-s3 driver (http) May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants