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> | 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
| import { AttachmentRecord, AttachmentState } from './Schema.js'; | ||
| import { SyncErrorHandler } from './SyncErrorHandler.js'; | ||
|
|
||
| export class StorageService { |
There was a problem hiding this comment.
For consistency, this should be called SyncingService
| 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.
| export class NodeFileSystemAdapter implements LocalStorageAdapter { | ||
| async initialize(): Promise<void> { | ||
| // const dir = this.getUserStorageDirectory(); | ||
| const dir = path.resolve('./user_data'); |
There was a problem hiding this comment.
Maybe allow the base directory to be specified as a constructor param and store it for use later.
|
|
||
| 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.
| watchAttachments, | ||
| logger, | ||
| tableName = ATTACHMENT_TABLE, | ||
| syncInterval = 30 * 1000, |
There was a problem hiding this comment.
We typically indicate the millisecond unit in the variable name
| syncInterval = 30 * 1000, | |
| syncIntervalMs= 30 * 1000, |
| 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.
| mediaType?: string; | ||
| metaData?: string; | ||
| id?: string; | ||
| }): Promise<AttachmentRecord> { |
There was a problem hiding this comment.
The updateHook callback used in other SDKs is important and should be specified here.
That hooks allows the saveFile operation to also associate the newly created attachment record with relational data. Failing to associate the record could result in racy archiving of the newly created record.
2 participants
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: