sor4chi / hono-storage

A storage helper for Hono. Support disk, memory, S3, etc.
MIT License
101 stars 3 forks source link

feat: signed url support for S3 / R2 #38

Closed sor4chi closed 9 months ago

sor4chi commented 10 months ago

close: #18

changeset-bot[bot] commented 10 months ago

🦋 Changeset detected

Latest commit: 1d21faedcc351df00d0864d8457d9c15f59bfeee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages | Name | Type | | -------------------------------- | ----- | | @hono-storage/s3 | Patch | | @hono-storage/core | Patch | | @hono-storage/r2-node-example | Patch | | @hono-storage/s3-node-example | Patch | | @hono-storage/s3-sign-example | Patch | | @hono-storage/s3-workers-example | Patch | | @hono-storage/memory | Patch | | @hono-storage/node-disk | Patch | | @hono-storage/memory-example | Patch | | @hono-storage/node-disk-example | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

sor4chi commented 10 months ago

Hi, @Code-Hex, @yusukebe! For now, I've tried to implement it in the simplest form I can think of. Sorry for the complexity of the code and commits, but if you don't mind, could you take a look at this PR?

yusukebe commented 10 months ago

LGTM!

Code-Hex commented 10 months ago

interface が完全に S3 に依存しているので、モックを作成する場合も S3 クライアントに依存してしまうのは残念な気がします。

GCS も R2 も S3 インターフェースに対応しているとはいえ、もう少し疎結合にできたりしないですか?

例えば Go 言語も interface に依存するものは少ない方がいいと推奨してたりします。ここでは 2つのメソッドを一つの interface に押し込んでるので、機能が増えるたびに interface の依存も増え、それに伴ってテストも修正する必要が出てきそうです。1 interface 1 method にできるとこの問題も解消できそうです。

これらのことを踏まえると以下の方針で修正できると良くなるのかなと思ってます🙏

cloudflare-pages[bot] commented 9 months ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1d21fae
Status:🚫  Build failed.

View logs

sor4chi commented 9 months ago

@Code-Hex コメントありがとうございます。色々忙しくて返答が大変遅くなってしまい申し訳ないです。

インターフェースの設計概念の件、そもそも機能ごとに分割するアイデアがなかったのでとても参考になりました。

ずっとこの分離に関して考えていたんですが、edgeなどのクライアントをリクエスト時にコンテキストから動的に初期化する必要があるため、どのように外部からclientを注入するかで迷った結果このようになりました。

リクエスト時にクライアントではなくrepositoryごと動的に初期化する設計にしてみましたがいかがでしょうか。