Update Storage SDK #139
Update Storage SDK #139
Conversation
🦋 Changeset detectedLatest commit: 3cb3361 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
joaquim-verges
left a comment
joaquim-verges
left a comment
There was a problem hiding this comment.
looks so much cleaner! excited for this
| await this.uploadProperties(file); | ||
| } | ||
| } | ||
| export function MockStorage(): ThirdwebStorage { |
There was a problem hiding this comment.
would it make sense to move this to the storage package? maybe as an internal class?
right now we have to duplicate this between solana / ts sdk
There was a problem hiding this comment.
Was thinking of doing this but left it since we had the duplicate from before. I think this generally makes sense - could be useful for other people testing?
Otherwise might be bleeding of our own implementations
| } | ||
| export function MockStorage(): ThirdwebStorage { | ||
| const uploader = new MockStorageUploader(); | ||
| const downloader = new MockStorageDownloader(); |
There was a problem hiding this comment.
btw your mapping doesn't need to be declared top level. can just create a new one in this method (I'd make it static too) and pass it in the mock uploader/downloader constructors. That way it's all nicely self contained
| "ipfs://": [ | ||
| "https://gateway.ipfscdn.io/ipfs/", | ||
| "https://cloudflare-ipfs.com/ipfs/", | ||
| "https://ipfs.io/ipfs/", |
There was a problem hiding this comment.
always found it error prone that you have to pass the /ipfs/ at the end. so easy to forget
There was a problem hiding this comment.
I'm thinking of this as a generic string match/replace - issue is that for other schemes, there's not necessarily an obvious thing to add on top like that - and would like the replacement to be as explicit as possible for the user.
Definitely might be nice to do some additional checks for the user though
| import { StorageDownloader } from "./downloaders/storage-downloader"; | ||
| import { IpfsUploader } from "./uploaders/ipfs-uploader"; | ||
|
|
||
| export class ThirdwebStorage<T extends UploadOptions = IpfsUploadBatchOptions> { |
There was a problem hiding this comment.
make sure to add some docs / examples
There was a problem hiding this comment.
Indeed - will put these in separate PR like Solana
|
|
||
| export type Json = JsonLiteral | FileOrBuffer | JsonObject | Json[]; | ||
|
|
||
| export type JsonObject = { [key: string]: Json }; |
There was a problem hiding this comment.
I think i duplicated these in solana, can you check that we're importing from here in both ts SDK/solana?
| await this.contractWrapper.getSigner()?.getAddress(), | ||
| { | ||
| rewriteFileNames: { | ||
| fileStartNumber: 0, |
|
@jnsdls any idea why the e2e react test would timeout on mount? |
Trying to debug it now - it doesn't like the |
|
Fixed! |
No description provided.