Update attachment package #735
Conversation
|
| this.logger = logger; | ||
| } | ||
|
|
||
| watchActiveAttachments(onUpdate: () => void): AbortController { |
There was a problem hiding this comment.
The AttachmentContext here is typically an exclusive access context (provided by the AttachmentsService). The context provides general methods for reading and writing AttachmentRecords in a safe context.
The watchActiveAttachments method does fall in to the category of reading Attachment records, but it does not typically require being executed in an exclusive context. It's usually only called once when the sync starts for initialisation. Furthermore, the handler in onUpdate typically will request an AttachmentContext in order to process the active attachments, which can produce a rather interesting code flow.
For consistency with other SDKs, I'd recommend moving this to the AttachmentService.
onUpdate should probably be an async handler.
Alternatively, we could use the Differentially Watched queries here. That would have the advantage of only triggering when there are actual changes to the result set. It also could make closing/aborting and listening for changes simpler.
watchActiveAttachments(): DifferentialWatchedQuery<AttachmentRecord> 
There was a problem hiding this comment.
I see, okay the separation makes sense - improved the consistency in the latest batch of commits. Also took the differential watched query approach.
| import { AbstractPowerSyncDatabase, ILogger, Transaction } from '@powersync/common'; | ||
| import { AttachmentRecord, AttachmentState, attachmentFromSql } from './Schema.js'; | ||
|
|
||
| export class AttachmentContext { |
There was a problem hiding this comment.
Just a reminder to add doc comments to all public classes and interfaces
There was a problem hiding this comment.
I added doc comments all of the public classes, functions and interfaces - should have total coverage now.
| import { SyncErrorHandler } from './SyncErrorHandler.js'; | ||
|
|
||
| export class StorageService { | ||
| context: AttachmentContext; |
There was a problem hiding this comment.
The SyncingService is usually instantiated when the attachment queue is initialised. The SyncingService then dynamically requests a lock on an AttachmentContext via the AttachmentService. This implementation should receive a reference to an AttachmentService here.
|
|
||
| async downloadAttachment(attachment: AttachmentRecord): Promise<AttachmentRecord> { | ||
| try { | ||
| const fileBlob = await this.remoteStorage.downloadFile(attachment); |
There was a problem hiding this comment.
I think we should have the local storage adapter support writing the same type as returned from the remote downloadFile method without a conversion here.
packages/attachments/src/storageAdapters/NodeFileSystemAdapter.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const config: UserConfigExport = { | ||
| plugins: [topLevelAwait()], | ||
| worker: { |
There was a problem hiding this comment.
An alternative could be to use the Node.js SDK for tests instead. That should require less config here.
| metaData, | ||
| id | ||
| }: { | ||
| data: ArrayBuffer | Blob | string; |
There was a problem hiding this comment.
It would be nice to export this to it's own dedicated type instead of inlining.
This a port of the Swift, Dart and Kotlin attachment's API to the JS SDK.
This addresses the following issues #715 and #714 by adding the
meta_datacolumn to the table and persisting it in the attachments table.It is currently still a work-in-progress so there are a couple things that still needs to be done: