feat: add uploadthing driver #390
Conversation
src/drivers/uploadthing.ts
Outdated
|
|
||
| const getUTFetch = () => { | ||
| return (utFetch ??= ofetch.create({ | ||
| baseURL: "https://uploadthing.com/api", |
There was a problem hiding this comment.
Would it makes sense to support an optional option for baseURL? (mainly to allow unit testing)
src/drivers/uploadthing.ts
Outdated
| }).then((res) => res.ok); | ||
| }, | ||
| getItem(key) { | ||
| return ofetch(`https://utfs.io/f/${key}`); |
There was a problem hiding this comment.
Maybe, somehow make it configurable too? (not sure about /api and this API difference but just a taught to again make testing easier)
There was a problem hiding this comment.
utfs is our cloudflare rewriter that handles caching and mapping keys to different regions.
uploadthing.com/api is where everything else goes.
can make it configurable for testing though!
src/drivers/uploadthing.ts
Outdated
| async clear() { | ||
| const client = getClient(); | ||
| const keys = await client.listFiles({}).then((r) => r.map((f) => f.key)); | ||
| return client.deleteFiles(keys).then(() => {}); |
There was a problem hiding this comment.
clear(base) is suported too. . I guess we might need to support it (client-side or server side) otherwise one call drops entire collection of uploads 🗡️
There was a problem hiding this comment.
yea 😅
not sure how though as currently we just have delete by key and all keys are random (no directories (yet))
|
@pi0 just tried to test this out and we stop usage of the utapi from client (mostly cause they shouldn't leak their api keys there) got any other good way to test this? oh right and our api also doesn't enable cors so it cannot be used from browser anyways :)
|
|
We have unit tests you can run with pnpm run vitest (unstoage also targets server). We mainly need to mock endpoints (responding from mobile if couldn’t find it please tell me to push commit) |
|
I just realized that in order to support this we'd need to allow setting the key manually. right now uploadthing generates the key for you |
src/drivers/uploadthing.ts
Outdated
| * Primarily used for testing | ||
| * @default "https://uploadthing.com/api" | ||
| */ | ||
| uploadthignApiUrl?: string; |
@pi0 is there a concept of mapping keys inside unstorage? For example when we setItem we get the key from UT, that we could then map to the unstorage key, i.e: storage.setItem("foo")
// internally map foo -> KEY_FROM_UT
storage.getItem("foo")
// key = map["foo"], fetch(utfs.io/f/key) |
|
I think we mainly need a storage for keys which is usually same storage layer. If your API is designed to always return random names, I am up to consider a new change that (in the meantime, i guess it would be also nice if you consider supporting custom names. let's see which comes first haha) |
The reason we don't is because names must be URL-safe, and have a length limit. |
|
Hmm, I see. What if we make the driver make sure that input is url-safe and respects the limit like any fs does (so it throws an error if it doesn't comply) I think users of the driver, would understandably follow this and probably can benefit more from such control vs random ids. Thoughts? |
|
So I did a thing to track internal vs uploadthing keys - not sure if it aligns with what this project aims to do though - I'll let you be the judge. Currently passing all tests except |
Merged fix for this on the API side |
src/drivers/uploadthing.ts
Outdated
| /** | ||
| * Primarily used for testing | ||
| * @default "https://uploadthing.com/api" | ||
| */ | ||
| uploadthignApiUrl?: string; | ||
| /** | ||
| * Primarily used for testing | ||
| * @default "https://utfs.io/f" | ||
| */ | ||
| uploadthingFileUrl?: string; |
There was a problem hiding this comment.
these makes no sense as we don't allow this override to the UTApi anyways
There was a problem hiding this comment.
This option provides two benefits:
- Unit testing the driver internally without actually calling UTAPi but mocking it in unstorage end-to-end tests (we use msw but it is not that reliable...)
- Allow users a way to Proxy your API if their server deployment requires this for any reason
I think both are internal and advanced cases. But if you really want to discourage users more to change them, we can definitely make it better with naming and JSDocs.
There was a problem hiding this comment.
Yes but that would only override the URL for the fetch calls that doesnt use utapi. All fetches made within utapi will still use uploadthing.com (although we have an internal env variable that overrides it)
|
current approach only works for a long-running server since it depends on that in-memory map. we're thinking about how to support user-provided keys |
|
Now just gonna have to get the mocking right 😅 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
+ Coverage 64.79% 66.05% +1.25%
==========================================
Files 41 41
Lines 4071 4186 +115
Branches 489 520 +31
==========================================
+ Hits 2638 2765 +127
+ Misses 1422 1411 -11
+ Partials 11 10 -1 ☔ View full report in Codecov by Sentry. |
|
struggling getting msw to intercept the requests 🤔
|
|
yeah. msw is not the best in class nor supports ofetch which I am aware of and also the fix but still hesitated because by doing it we add an overhead to every user of it only because of msw. I will try to check it ASAP. |
✅ Live Preview ready!
|
|
@pi0 updated to use native custom ids now. there are some tests failing however that's expected, since they relate to deletion of files. UT is not a KV store, so when a file is deleted in UT, it is not instantly deleted but it may take up to 30 seconds before it is deleted at the storage provider, and then deleted from our db. some of the tests here assume that calling idk how you wanna pursue given that knowledge. alter the tests? custom test suite for UT? cc @markflorkowski we could, delete the customId field when a file is marked for deletion, so that a new file may be uploaded with that same customId even before the file is removed. idk if that would make sense for us to do, but it would be a workaround to fix this issue another thing that we could do is to filter out the files marked for deletion in the |
| @@ -231,7 +231,7 @@ export function createStorage<T extends StorageValue>( | |||
| async setItems(items, commonOptions) { | |||
| await runBatch(items, commonOptions, async (batch) => { | |||
| if (batch.driver.setItems) { | |||
| await asyncCall( | |||
| return asyncCall( | |||
There was a problem hiding this comment.
fairly certain this was a bug before. you called setItems and then setItem for every file so files were getting uploaded twice.
noticed this since the newly introduced customids on uploadthing must be unique and we throw if that's not the case and we got duplicate requests when testing the setItems function before i changed the keyword here
There was a problem hiding this comment.
Nice catch. Do you mind splitting it into a new PR as well? 🙏🏼
|
Exciting updates 🚀 Sorry i'm quite busy still will try to come to this PR as soon as could also for failing tests. |
| @@ -92,6 +92,7 @@ | |||
| "types-cloudflare-worker": "^1.2.0", | |||
| "typescript": "^5.3.3", | |||
| "unbuild": "^2.0.0", | |||
| "uploadthing": "6.3.2-canary.a35b49f", | |||
There was a problem hiding this comment.
| "uploadthing": "6.3.2-canary.a35b49f", | |
| "uploadthing": "6.4.0", |
| @@ -108,7 +109,8 @@ | |||
| "@planetscale/database": "^1.13.0", | |||
| "@upstash/redis": "^1.28.1", | |||
| "@vercel/kv": "^0.2.4", | |||
| "idb-keyval": "^6.2.1" | |||
| "idb-keyval": "^6.2.1", | |||
| "uploadthing": "^6.0.0" | |||
There was a problem hiding this comment.
| "uploadthing": "^6.0.0" | |
| "uploadthing": "^6.4.0" |
🔗 Linked issue
#389
❓ Type of change
📚 Description
📝 Checklist