feat: $blobs.getUrl and $blobs.create methods #184
Conversation
There was a problem hiding this comment.
Progress looks good, I made some comments.
How about keeping these methods (create() and getUrl()) in a separate class BlobApi that takes the blobStore as a constructor param? It feels like we're overloading the blobStore API here with something that has a different purpose.
|
I added a couple of commits that I think addresses everything but getting the port from fastify into the BlobApi class / getUrl method. Added a comment above about that. |
…, pass BlobStore to BlobApi, add tests
src/blob-api.js
Outdated
| * Write blobs for provided variants of a file | ||
| * @param {{ original: string, preview?: string, thumbnail?: string }} filepaths | ||
| * @param {{ mimeType: string }} metadata | ||
| * @returns {Promise<{ original: Omit<BlobId, 'driveId'>, preview?: Omit<BlobId, 'driveId'>, thumbnail?: Omit<BlobId, 'driveId'> }>} |
There was a problem hiding this comment.
This will actually need to return type Observation['attachments'][number] e.g.
{ driveId: string, name: string, type: 'photo' | 'video' | 'audio' }
There was a problem hiding this comment.
so far this module hasn't known about driveId. Would that be passed in the create method?
There was a problem hiding this comment.
Ah very good point. It's actually a property on the writeStream e.g. blobStore.createWriteStream().driveId although I just realised that was not documented (my bad) and it's an awkward API too. Since the driveId for any written blobs will never change for a particular instance of the blobStore, I think it makes sense to just expose it as blobStore.writerDriveId, and use it from there. I've created an issue and made a fix
There was a problem hiding this comment.
Great work on getting this wrapped up, looks good to merge now once the driveId question is resolved. I'll merge #219 today and then you can use blobStore.writerDriveId instead of metadata.driveId. Once that is done we can merge.
@sethvincent I saw another review request for me, but this comment still doesn't seem to be resolved? |
* main: (25 commits) add initial implementation of MemberApi (#232) feat: $blobs.getUrl and $blobs.create methods (#184) chore: update manager e2e tests (#237) feat: add capabilities (#231) feat: coreOwnership integration [3/3] (#230) feat: CoreOwnership class w getOwner & getCoreKey [2/3] (#229) feat: handle `coreOwnership` records in `IndexWriter` [1/3] (#214) fix: adjust storage options for MapeoManager and MapeoProject (#235) implement addProject method for MapeoManager class (#215) implement listProjects method for MapeoManager class (#208) feat: expose blobStore.writerDriveId (#219) implement wrapper client containing createProject and getProject methods (#199) add project settings functionality to MapeoProject (#187) feat: Add encode/decode for project keys [3/3] (#203) feat: update protobuf for RPC [2/3] (#202) chore: move protobuf messages into src/generated [1/3] (#201) feat: Add internal `dataType.createWithDocId()` (#192) chore: explicitly set "mode" opt for encryptionKeys column creation (#186) chore: fix linting and type checking (#183) chore: consolidate encryption key columns in projectKeys table (#181) ...
closes #104
There are a couple todos that need to be addressed that I'm not sure about. See comment below.