feat(attachments): streaming attachment transport + saveFileFromUri#1039
feat(attachments): streaming attachment transport + saveFileFromUri#1039khawarizmus wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 16c6a3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
Model attachment byte-transfer as a single operation via AttachmentTransportAdapter, with a default BufferedAttachmentTransport that composes the existing local/remote adapters. Add AttachmentQueue.saveFileFromUri and an optional LocalStorageAdapter.moveFile for buffer-free registration of files already on disk, a native Expo ExpoFileSystemTransportAdapter, and moveFile in the Node and React Native local adapters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
509d298 to
bf2d8cc
Compare
…pter Native streaming transport for @dr.pogodin/react-native-fs (raw binary PUT via uploadFiles + downloadFile), mirroring ExpoFileSystemTransportAdapter, so non-Expo apps can transfer large attachments without buffering them in JS memory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pter Enhance AttachmentTransportAdapter to support file deletion operations. This includes updates to ExpoFileSystemTransportAdapter and ReactNativeFSTransportAdapter to implement the new deleteFile method. Additionally, adjustments were made to AttachmentQueue and BufferedAttachmentTransport to integrate the delete functionality into the attachment synchronization process.
…vulnerability audit This commit updates the websocket-driver dependency to version 0.7.5 in both pnpm-lock.yaml and pnpm-workspace.yaml to address security advisories. Additionally, the README.md for attachments-storage-react-native has been enhanced to clarify the functionality of local storage and streaming transport adapters, including usage examples and API details.
simolus3
left a comment
simolus3
left a comment
There was a problem hiding this comment.
I think the changes and the BufferedAttachmentTransport fallback are clean, but we can probably restructure the public interface a bit to get rid of runtime errors.
| * Describes the HTTP request used to upload a file's bytes to remote storage. | ||
| * Typically points at a presigned URL. | ||
| */ | ||
| export interface ExpoUploadRequest { |
There was a problem hiding this comment.
Would it be possible to add an optional dependency on expo and then use expo types here?
| * @experimental | ||
| * @alpha This is currently experimental and may change without a major version bump. |
There was a problem hiding this comment.
Do we even need to export this from common? I assume users wouldn't construct this directly, they'd rely on this being the default right?
| * default {@link BufferedAttachmentTransport}, which delegates all three operations | ||
| * to it. | ||
| * | ||
| * Required unless a {@link AttachmentQueueOptions.transportAdapter} is provided. If |
There was a problem hiding this comment.
Can we encode this requirement in types? E.g. we could have export type AttachmentQueueOptions = { ... remaining fields ... } & ({ remoteStorage: RemoteStorageAdapter } | { transportAdapter: AttachmentTransportAdapter })
| /** Adapter for remote file storage operations. Undefined when a custom `transportAdapter` is used. */ | ||
| readonly remoteStorage?: RemoteStorageAdapter; |
There was a problem hiding this comment.
remoteStorage is not used anywhere anymore. Given that attachments are still in beta and we're about to make a breaking change, let's just remove the field.
| this.logger = logger ?? db.logger; | ||
| this.attachmentService = new AttachmentService(db, this.logger, tableName, archivedCacheLimit); | ||
|
|
||
| if (!transportAdapter && !remoteStorage) { |
There was a problem hiding this comment.
We should also throw when both are set (or better yet make it a type error), because the remote implementation wouldn't be used in that case which sounds unintentional.
| * @returns Promise resolving to the created attachment record | ||
| * @throws Error if the local storage adapter does not support moving files | ||
| */ | ||
| async saveFileFromUri({ |
There was a problem hiding this comment.
Can we move common options into a shared interface and introduce a private method to implement saveFile and saveFileFromUri given that they're nearly the same?
| updateHook?: (transaction: Transaction, attachment: AttachmentRecord) => Promise<void>; | ||
| }): Promise<AttachmentRecord> { | ||
| if (!this.localStorage.moveFile) { | ||
| throw new Error('The configured local storage adapter does not support moveFile, required by saveFileFromUri.'); |
There was a problem hiding this comment.
I would ideally also like this to be a static error.
I wonder if there's a way to make support for streaming downloads a subinterface of LocalStorageAdapter, and add a type parameter to AttachmentQueue to declare the type of local storage adapter used. Then we could use conditional types to declare that saveFileFromUri is only available when a stream-aware local adapter is used.
Additionally, it would also help with the option case above (we could require a RemoteAdapter if the local storage adapter is non-streaming only). Essentially, AttachmentTransportAdapter would then be a subinterface of LocalStorageAdapter and we'd add moveFile to it only.
2 participants
Summary
Addresses the streaming request in discussion #968. The
AttachmentQueuecurrently round-trips every file body through anArrayBufferin JS memory (localStorage.readFile→remoteStorage.uploadFile, and the reverse on download), which causes memory pressure / OOM for large files on lower-end devices.This models the remote side (transfer and delete) as a single adapter so implementations can stream file-URI → network natively, never materializing the file in the JS heap.
What's included
AttachmentTransportAdapter(common) — owns all remote operations:upload/download/delete.BufferedAttachmentTransport— the default, composing the existing local + remote adapters (delegates upload/download/delete toremoteStorage). Used automatically when notransportAdapteris provided, so existing setups are unchanged and backward compatible.AttachmentQueue— new optionaltransportAdapter;remoteStorageis now optional (required only when no transport is given — a transport fully replaces it). NewsaveFileFromUrito register a file already on disk without reading it into memory.LocalStorageAdapter.moveFile?(optional) — buffer-free relocation into managed storage; implemented in the Node, Expo, and RN-FS adapters.ExpoFileSystemTransportAdapter(attachments-storage-react-native) — native transport using Expo'suploadAsync/downloadAsync(+ adeleteFilecallback) for buffer-free transfer.ReactNativeFSTransportAdapter(attachments-storage-react-native) — native transport using@dr.pogodin/react-native-fs'suploadFiles/downloadFile(+ adeleteFilecallback).Backward compatibility
Default behavior is unchanged — the buffered transport is composed from the existing
localStorage/remoteStoragewhen no transport is supplied, andRemoteStorageAdapteris unchanged.remoteStoragebecoming optional is a widening, so existing{ localStorage, remoteStorage }setups still compile and behave identically. A customtransportAdapter, when supplied, handles all remote operations and anyremoteStoragepassed alongside it is ignored.One intended behavior change for the
RemoteStorageAdapter:downloadFilenow receives the record with its destinationlocalUripopulated (previouslynullfor fresh downloads). Harmless for real adapters, which key offfilename.Generated with Claude, tested and reviewed manually.