feat: MemoryDriver persistence — file/localStorage adapters#816
feat: MemoryDriver persistence — file/localStorage adapters#816
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…stom adapter union type Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… driver integration and tests Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…casts, simplify unref Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ctstack/data/ Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds opt-in persistence to the in-memory driver (spec + implementation), enabling state to survive Node.js restarts (file) and browser refreshes (localStorage) while keeping the default “pure memory” behavior unchanged.
Changes:
- Expanded the spec (
MemoryPersistenceConfigSchema) into a union supporting shorthand ('file' | 'local'), typed objects, and custom adapters. - Implemented persistence lifecycle in
InMemoryDriver(load onconnect(), dirty tracking on mutations, flush ondisconnect(), plus a publicflush()). - Added Node.js file adapter + browser localStorage adapter, plus tests and ignore rules for the default data directory.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/data/driver/memory.zod.ts | Redefines persistence config as a union and introduces adapter/config schemas |
| packages/spec/src/data/driver/memory.test.ts | Updates/extends schema tests for new persistence shapes/defaults |
| packages/plugins/driver-memory/src/memory-driver.ts | Wires persistence into driver lifecycle + marks mutations dirty + adds flush() |
| packages/plugins/driver-memory/src/persistence/file-adapter.ts | New file-based persistence adapter with atomic write + autosave timer |
| packages/plugins/driver-memory/src/persistence/local-storage-adapter.ts | New localStorage-based persistence adapter |
| packages/plugins/driver-memory/src/persistence/index.ts | Barrel exports for persistence adapters |
| packages/plugins/driver-memory/src/persistence/persistence.test.ts | Adds persistence tests (file + custom adapter + no persistence) |
| packages/plugins/driver-memory/src/index.ts | Updates package exports (driver + types + adapters) |
| .gitignore | Ignores .objectstack/data/ persistence output directory |
| ROADMAP.md | Marks in-memory persistence as completed |
Comments suppressed due to low confidence (1)
packages/plugins/driver-memory/src/memory-driver.ts:168
- With persistence enabled,
connect()always appliesinitialDataafter restoring persisted state, which can duplicate seed records on every restart. Also, seeding viainitialDatadoesn’t callmarkDirty(), so a fresh seeded DB won’t be persisted unless a later mutation happens. Consider only applyinginitialDatawhen no persisted state is loaded (or only for missing tables) and marking dirty after seeding.
// Load initial data if provided
if (this.config.initialData) {
for (const [objectName, records] of Object.entries(this.config.initialData)) {
const table = this.getTable(objectName);
for (const record of records) {
| async load(): Promise<Record<string, any[]> | null> { | ||
| try { | ||
| const raw = localStorage.getItem(this.storageKey); | ||
| if (!raw) return null; | ||
| return JSON.parse(raw) as Record<string, any[]>; |
There was a problem hiding this comment.
This adapter is documented as “Browser only — will throw if used in non-browser environments,” but load() currently swallows ReferenceError/security errors and returns null. Add an explicit localStorage availability check (and throw a clear error) to avoid silent data loss when persistence: 'local' is used outside a browser or when storage access is blocked.
| export class LocalStoragePersistenceAdapter { | ||
| private readonly storageKey: string; | ||
| private static readonly SIZE_WARNING_BYTES = 4.5 * 1024 * 1024; // 4.5MB warning threshold | ||
|
|
||
| constructor(options?: { key?: string }) { |
There was a problem hiding this comment.
LocalStorage persistence isn’t covered by tests in this PR (persistence tests cover file + custom adapter only). Consider adding a vitest case (jsdom env or a globalThis.localStorage mock) to validate save/load behavior for the browser adapter.
| if (typeof persistence === 'string') { | ||
| if (persistence === 'file') { | ||
| const { FileSystemPersistenceAdapter } = await import('./persistence/file-adapter.js'); | ||
| this.persistenceAdapter = new FileSystemPersistenceAdapter(); | ||
| } else if (persistence === 'local') { |
There was a problem hiding this comment.
When persistence is a string, adapters are imported/initialized without checking whether the current runtime supports them. Add explicit environment checks before selecting/importing file vs local so persistence: 'local' in Node and persistence: 'file' in browsers fail with a clear, actionable error message (instead of silent no-ops or import errors).
| if (this.dirty && this.currentDb) { | ||
| await this.writeToDisk(this.currentDb); | ||
| this.dirty = false; |
There was a problem hiding this comment.
The setInterval(async () => ...) auto-save callback doesn’t handle write errors. If writeToDisk() throws (permissions, disk full, invalid path), the rejected promise can surface as an unhandled rejection. Wrap the interval body in try/catch and surface failures (e.g. via logging) so persistence failures are observable.
| if (this.dirty && this.currentDb) { | |
| await this.writeToDisk(this.currentDb); | |
| this.dirty = false; | |
| try { | |
| if (this.dirty && this.currentDb) { | |
| await this.writeToDisk(this.currentDb); | |
| this.dirty = false; | |
| } | |
| } catch (error) { | |
| // Surface auto-save persistence failures without crashing the process | |
| console.error( | |
| '[FileSystemPersistenceAdapter] Auto-save failed for', | |
| this.filePath, | |
| error, | |
| ); |
| */ | ||
| private markDirty(): void { | ||
| if (this.persistenceAdapter) { | ||
| this.persistenceAdapter.save(this.db); |
There was a problem hiding this comment.
markDirty() calls the async persistenceAdapter.save() without awaiting or handling rejections. If a custom adapter rejects (or JSON.stringify fails in the localStorage adapter), this can surface as an unhandled promise rejection. Consider firing-and-forgetting with explicit rejection handling (e.g., void save(...).catch(...)) or queueing/throttling saves with error logging via the driver logger.
| this.persistenceAdapter.save(this.db); | |
| void this.persistenceAdapter.save(this.db).catch((error) => { | |
| this.logger.error('MemoryDriver persistence save failed', { error }); | |
| }); |
| persistence?: string | { | ||
| type?: 'file' | 'local'; | ||
| path?: string; | ||
| key?: string; | ||
| autoSaveInterval?: number; | ||
| adapter?: PersistenceAdapterInterface; | ||
| }; |
There was a problem hiding this comment.
The object form of persistence uses type?: 'file' | 'local', but object configs are only meaningful when type is present (per the spec union). With type?, callers can pass an object without type and persistence will be silently ignored. Make type required for the object variants (and/or use a discriminated union) and throw on unsupported shapes.
| persistence?: string | { | |
| type?: 'file' | 'local'; | |
| path?: string; | |
| key?: string; | |
| autoSaveInterval?: number; | |
| adapter?: PersistenceAdapterInterface; | |
| }; | |
| persistence?: | |
| | 'file' | |
| | 'local' | |
| | { | |
| /** File-system persistence adapter with options */ | |
| type: 'file'; | |
| path?: string; | |
| autoSaveInterval?: number; | |
| /** Not applicable for file persistence */ | |
| key?: never; | |
| /** Not applicable when using built-in adapters */ | |
| adapter?: never; | |
| } | |
| | { | |
| /** localStorage persistence adapter with options */ | |
| type: 'local'; | |
| key?: string; | |
| /** Not applicable for local persistence */ | |
| path?: never; | |
| autoSaveInterval?: never; | |
| /** Not applicable when using built-in adapters */ | |
| adapter?: never; | |
| } | |
| | { | |
| /** Custom persistence adapter implementation */ | |
| adapter: PersistenceAdapterInterface; | |
| /** Disallow built-in adapter discriminator on custom adapter configs */ | |
| type?: never; | |
| path?: never; | |
| key?: never; | |
| autoSaveInterval?: never; | |
| }; |
| * - `{ type: 'file', path?: string }` — File-system with custom path | ||
| * - `{ type: 'local', key?: string }` — localStorage with custom key | ||
| * - `{ adapter: PersistenceAdapter }` — Custom adapter | ||
| * |
There was a problem hiding this comment.
The JSDoc examples for the file object variant omit autoSaveInterval, but the schema supports it (with a default). Update the docs to reflect the full shape (e.g. { type: 'file', path?: string, autoSaveInterval?: number }) so config examples match actual accepted options.
| * | ||
| * - `'file'`: Persist to disk file (Node.js only, default path: `.objectstack/data/memory-driver.json`) | ||
| * - `'local'`: Persist to localStorage (Browser only, default key: `objectstack:memory-db`) | ||
| * - `{ type: 'file', path?: string }`: File-system with custom path |
There was a problem hiding this comment.
In the MemoryConfigSchema property docs, the file object variant is described as { type: 'file', path?: string }, but autoSaveInterval is also supported. Consider updating this line to include autoSaveInterval?: number for consistency with the schema and earlier docs.
| * - `{ type: 'file', path?: string }`: File-system with custom path | |
| * - `{ type: 'file', path?: string, autoSaveInterval?: number }`: File-system with custom path and auto-save interval |
| export { FileSystemPersistenceAdapter } from './persistence/file-adapter.js'; | ||
| export { LocalStoragePersistenceAdapter } from './persistence/local-storage-adapter.js'; | ||
|
|
There was a problem hiding this comment.
Re-exporting FileSystemPersistenceAdapter from the package root forces consumers (including browser bundles) to resolve ./persistence/file-adapter.js, which imports node:fs/node:path. This undermines the dynamic-import strategy and will break browser/edge builds even when file persistence isn’t used. Consider removing these root exports and instead exposing adapters via environment-specific/conditional exports (or separate entrypoints) so browser imports don’t pull in Node-only modules.
| export { FileSystemPersistenceAdapter } from './persistence/file-adapter.js'; | |
| export { LocalStoragePersistenceAdapter } from './persistence/local-storage-adapter.js'; |
| } else if ('type' in persistence) { | ||
| if (persistence.type === 'file') { | ||
| const { FileSystemPersistenceAdapter } = await import('./persistence/file-adapter.js'); | ||
| this.persistenceAdapter = new FileSystemPersistenceAdapter({ | ||
| path: persistence.path, |
There was a problem hiding this comment.
In the object-config branch, an unexpected persistence.type value currently results in no adapter being set (and no error). Add an explicit else that throws for unknown type values so misconfiguration doesn’t silently disable persistence.
The in-memory driver loses all data on process restart or page refresh. This adds opt-in persistence via disk file (Node.js) or localStorage (browser), with zero breaking changes to existing behavior.
Spec (
packages/spec/src/data/driver/memory.zod.ts)MemoryPersistenceConfigSchemaas a union type:'file'|'local'{ type: 'file', path?, autoSaveInterval? }|{ type: 'local', key? }{ adapter: PersistenceAdapter }PersistenceAdapterSchema,FilePersistenceConfigSchema,LocalStoragePersistenceConfigSchema,CustomPersistenceConfigSchemaDriver (
packages/plugins/driver-memory/)FileSystemPersistenceAdapter— atomic write (temp+rename), dirty-flag auto-save timer, configurable path (default.objectstack/data/memory-driver.json)LocalStoragePersistenceAdapter— synchronous save with 5MB warning, configurable key (defaultobjectstack:memory-db)InMemoryDriver— persistence lifecycle wired intoconnect()(load), CRUD mutations (markDirty),disconnect()(flush). Publicflush()method. Adapters loaded via dynamic import to avoid cross-environment issues.Other
.gitignore:**/.objectstack/data/ROADMAP.md: marked persistence completeOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.