diff --git a/src/observableInterfaces.ts b/src/observableInterfaces.ts index 0ec04dc1..ef1b1ead 100644 --- a/src/observableInterfaces.ts +++ b/src/observableInterfaces.ts @@ -161,6 +161,7 @@ export interface UpdateFnParams { value: T; mode?: GetMode; lastSync?: number | undefined; + changes?: Change[]; } export interface UpdateSetFnParams extends UpdateFnParams { lastSync?: never; diff --git a/src/sync-plugins/crud.ts b/src/sync-plugins/crud.ts index 0cc4482f..5509542a 100644 --- a/src/sync-plugins/crud.ts +++ b/src/sync-plugins/crud.ts @@ -1,4 +1,5 @@ import { + Change, ObservableEvent, ObservableParam, RetryOptions, @@ -141,8 +142,9 @@ function retrySet( action: 'create' | 'update' | 'delete', itemKey: string, itemValue: any, + change: Change, actionFn: (value: any, params: SyncedSetParams) => Promise, - saveResult: (itemKey: string, itemValue: any, result: any, isCreate: boolean) => void, + saveResult: (itemKey: string, itemValue: any, result: any, isCreate: boolean, change: Change) => void, ) { // If delete then remove from create/update, and vice versa if (action === 'delete') { @@ -166,10 +168,12 @@ function retrySet( queuedRetries[action].set(itemKey, itemValue); - return runWithRetry(params, retry, 'create_' + itemKey, () => - actionFn!(itemValue, params).then((result) => { + const paramsWithChanges: SyncedSetParams = { ...params, changes: [change] }; + + return runWithRetry(paramsWithChanges, retry, 'create_' + itemKey, () => + actionFn!(itemValue, paramsWithChanges).then((result) => { queuedRetries[action]!.delete(itemKey); - return saveResult(itemKey, itemValue, result as any, true); + return saveResult(itemKey, itemValue, result as any, true, change); }), ); } @@ -353,6 +357,7 @@ export function syncedCrud(); const updates = new Map(); const deletes = new Set(); + const changesById = new Map(); const getUpdateValue = (itemValue: object, prev: object) => { return updatePartial @@ -373,6 +378,7 @@ export function syncedCrud, isCreate: boolean, + change: Change, ) => { if (data) { let saved: Partial = ( @@ -525,7 +533,7 @@ export function syncedCrud { - pendingCreates.delete(itemKey); - }, - ); + return retrySet( + params, + retry, + 'create', + itemKey, + createObj, + changesById.get(itemKey)!, + createFn!, + saveResult, + ).then(() => { + pendingCreates.delete(itemKey); + }); }), // Handle updates @@ -576,7 +592,16 @@ export function syncedCrud 0) { - return retrySet(params, retry, 'update', itemKey, changed, updateFn!, saveResult); + return retrySet( + params, + retry, + 'update', + itemKey, + changed, + changesById.get(itemKey)!, + updateFn!, + saveResult, + ); } }), @@ -587,9 +612,9 @@ export function syncedCrud( // Save metadata const oldMetadata: PersistMetadata | undefined = metadatas.get(value$); - const { lastSync } = newMetadata; + const { lastSync } = newMetadata!; const metadata = Object.assign({}, oldMetadata, newMetadata); metadatas.set(value$, metadata); @@ -196,10 +196,10 @@ function updateMetadata( if (localState.timeoutSaveMetadata) { clearTimeout(localState.timeoutSaveMetadata); } - localState.timeoutSaveMetadata = setTimeout( - () => updateMetadataImmediate(value$, localState, syncState, syncOptions as SyncedOptions, newMetadata), - 0, - ); + metadatas.set(value$, { ...(metadatas.get(value$) || {}), ...newMetadata }); + localState.timeoutSaveMetadata = setTimeout(() => { + updateMetadataImmediate(value$, localState, syncState, syncOptions as SyncedOptions, metadatas.get(value$)!); + }, 0); } interface QueuedChange { @@ -660,10 +660,11 @@ async function doChangeRemote(changeInfo: PreppedChangeRemote | undefined) { onError: onSetError, update: (params: UpdateSetFnParams) => { if (updateResult) { - const { value, mode } = params; + const { value, mode, changes } = params; updateResult = { value: deepMerge(updateResult.value, value), mode: mode, + changes: changes ? [...(updateResult.changes || []), ...changes] : updateResult.changes, }; } else { updateResult = params; @@ -688,12 +689,16 @@ async function doChangeRemote(changeInfo: PreppedChangeRemote | undefined) { }); } - if (!didError) { + // If the plugin set which changes saved successfully then use those. + // Or if it didn't error then use all the changes + if (!didError || (updateResult as unknown as UpdateSetFnParams)?.changes) { // If this remote save changed anything then update cache and metadata // Because save happens after a timeout and they're batched together, some calls to save will // return saved data and others won't, so those can be ignored. - const pathStrs = Array.from(new Set(changesRemote.map((change) => change.pathStr))); - const { value: changes } = updateResult! || {}; + const { value: updateValue, changes: updateChanges = changesRemote } = updateResult! || {}; + const pathStrs = Array.from( + new Set((updateChanges as ChangeWithPathStr[]).map((change) => change.pathStr)), + ); if (pathStrs.length > 0) { let transformedChanges: object | undefined = undefined; const metadata: PersistMetadata = {}; @@ -720,8 +725,8 @@ async function doChangeRemote(changeInfo: PreppedChangeRemote | undefined) { // Remote can optionally have data that needs to be merged back into the observable, // for example Firebase may update dateModified with the server timestamp - if (changes && !isEmpty(changes)) { - transformedChanges = transformLoadData(changes, syncOptions, false, 'set'); + if (updateValue && !isEmpty(updateValue)) { + transformedChanges = transformLoadData(updateValue, syncOptions, false, 'set'); } if (transformedChanges !== undefined) { diff --git a/tests/keel.test.ts b/tests/keel.test.ts index a9da8361..8c4b6995 100644 --- a/tests/keel.test.ts +++ b/tests/keel.test.ts @@ -1,9 +1,9 @@ import { observable } from '@legendapp/state'; import { syncedKeel as syncedKeelOrig } from '../src/sync-plugins/keel'; import { configureSynced } from '../src/sync/configureSynced'; -import { promiseTimeout } from './testglobals'; +import { getPersistName, localStorage, ObservablePersistLocalStorage, promiseTimeout } from './testglobals'; -type APIError = { type: string; message: string; requestId?: string }; +type APIError = { type: string; message: string; requestId?: string; error?: Error }; type APIResult = Result; @@ -517,4 +517,114 @@ describe('keel', () => { }, ]); }); + test('Error handling in crud ', async () => { + const persistName = getPersistName(); + let errorAtOnError: Error | undefined = undefined; + let numErrors = 0; + const obs$ = observable( + syncedKeel({ + list: async () => fakeKeelList([{ ...ItemBasicValue(), other: 2, another: 3 }]), + create: async (): Promise => { + return { error: { message: 'test' } }; + }, + update: async ({ where }): Promise => { + return { data: { ...obs$[where.id].peek(), updatedAt: 2 } } as any; + }, + onError: (error) => { + numErrors++; + errorAtOnError = error; + }, + changesSince: 'last-sync', + persist: { + name: persistName, + plugin: ObservablePersistLocalStorage, + retrySync: true, + }, + }), + ); + + expect(obs$.get()).toEqual(undefined); + + await promiseTimeout(1); + + expect(obs$.get()).toEqual({ + id1: { id: 'id1', test: 'hi', other: 2, another: 3, createdAt: 1, updatedAt: 1 }, + }); + + obs$.id1.test.set('hello'); + obs$.id2.set({ id: 'id2', test: 'hi', other: 3, another: 4 }); + + await promiseTimeout(1); + + expect(errorAtOnError).toEqual(new Error('test')); + expect(numErrors).toEqual(1); + + expect(obs$.get()).toEqual({ + id1: { + id: 'id1', + test: 'hello', + other: 2, + another: 3, + createdAt: 1, + updatedAt: 2, + }, + id2: { id: 'id2', test: 'hi', other: 3, another: 4 }, + }); + await promiseTimeout(10); + + expect(localStorage.getItem(persistName + '__m')!).toEqual( + JSON.stringify({ + lastSync: 1, + pending: { id2: { p: null, t: ['object'], v: { id: 'id2', test: 'hi', other: 3, another: 4 } } }, + }), + ); + }); + test('onError reverts only one change if multiple fails', async () => { + let errorAtOnError: Error | undefined = undefined; + let numErrors = 0; + const obs$ = observable( + syncedKeel({ + list: async () => fakeKeelList([{ ...ItemBasicValue(), other: 2, another: 3 }]), + create: async (): Promise => { + return { error: { message: 'test' } }; + }, + update: async ({ where }): Promise => { + return { data: { ...obs$[where.id].peek(), updatedAt: 2 } } as any; + }, + onError: (error, params) => { + numErrors++; + errorAtOnError = error; + params.revert!(); + }, + }), + ); + + expect(obs$.get()).toEqual(undefined); + + await promiseTimeout(1); + + expect(obs$.get()).toEqual({ + id1: { id: 'id1', test: 'hi', other: 2, another: 3, createdAt: 1, updatedAt: 1 }, + }); + + obs$.id1.test.set('hello'); + obs$.id2.set({ id: 'id2', test: 'hi', other: 3, another: 4 }); + + await promiseTimeout(1); + + expect(errorAtOnError).toEqual(new Error('test')); + expect(numErrors).toEqual(1); + + expect(obs$.get()).toEqual({ + id1: { + id: 'id1', + test: 'hello', + other: 2, + another: 3, + createdAt: 1, + updatedAt: 2, + }, + id2: undefined, + }); + }); });