Skip to content

Commit

Permalink
Pass the changes through the saving process so that it can clear pend…
Browse files Browse the repository at this point in the history 
…ing for only the saves that changed and so revert can revert only the one errored change
  • Loading branch information
@jmeistrich
jmeistrich committed Nov 2, 2024
1 parent 4de06f3 commit 21b0af5
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 37 deletions.
1 change: 1 addition & 0 deletions src/observableInterfaces.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export interface UpdateFnParams<T = any> {
value: T;
mode?: GetMode;
lastSync?: number | undefined;
changes?: Change[];
}
export interface UpdateSetFnParams<T = any> extends UpdateFnParams<T> {
lastSync?: never;
Expand Down
75 changes: 51 additions & 24 deletions src/sync-plugins/crud.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
Change,
ObservableEvent,
ObservableParam,
RetryOptions,
Expand Down Expand Up @@ -141,8 +142,9 @@ function retrySet(
action: 'create' | 'update' | 'delete',
itemKey: string,
itemValue: any,
change: Change,
actionFn: (value: any, params: SyncedSetParams<any>) => Promise<any>,
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') {
Expand All @@ -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<any> = { ...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);
}),
);
}
Expand Down Expand Up @@ -353,6 +357,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
const creates = new Map<string, TLocal>();
const updates = new Map<string, object>();
const deletes = new Set<TRemote>();
const changesById = new Map<string, Change>();

const getUpdateValue = (itemValue: object, prev: object) => {
return updatePartial
Expand All @@ -373,6 +378,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
id = ensureId(value, fieldId, generateId);
}
if (id) {
changesById.set(id, change);
if (pendingCreates.has(id)) {
isCreate = false;
}
Expand All @@ -397,6 +403,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
}
} else if (path.length === 0) {
deletes.add(prevAtPath);
changesById.set(prevAtPath[fieldId], change);
}
} else {
// key, value, previous
Expand Down Expand Up @@ -427,6 +434,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
if (!itemValue) {
if (path.length === 1 && prevAtPath) {
deletes.add(prevAtPath);
changesById.set(prevAtPath[fieldId], change);
}
} else {
const previous = setAtPath(
Expand Down Expand Up @@ -455,19 +463,18 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
console.error('[legend-state]: added item without an id');
}
if (createFn) {
pendingCreates.add(item[fieldId]);
creates.set(item[fieldId], item);
const id = item[fieldId];
changesById.set(id, change);
pendingCreates.add(id);
creates.set(id, item);
} else {
console.warn('[legend-state] missing create function');
}
} else {
if (updateFn) {
updates.set(
item[fieldId],
updates.has(item[fieldId])
? Object.assign(updates.get(item[fieldId])!, item)
: item,
);
const id = item[fieldId];
changesById.set(id, change);
updates.set(id, updates.has(id) ? Object.assign(updates.get(id)!, item) : item);
} else {
console.warn('[legend-state] missing update function');
}
Expand All @@ -481,6 +488,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
input: TRemote,
data: CrudResult<TRemote>,
isCreate: boolean,
change: Change,
) => {
if (data) {
let saved: Partial<TLocal> = (
Expand Down Expand Up @@ -525,7 +533,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
// value is already the new value, can ignore
(saved as any)[key] === c ||
// user has changed local value
(key !== fieldId && i !== c)
(key !== fieldId && i !== undefined && i !== c)
) {
delete (saved as any)[key];
}
Expand All @@ -549,6 +557,7 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
update({
value,
mode: 'merge',
changes: [change],
});
}
}
Expand All @@ -562,11 +571,18 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
await waitForSet(waitForSetParam as any, changes, itemValue, { type: 'create' });
}
const createObj = (await transformOut(itemValue as any, transform?.save)) as TRemote;
return retrySet(params, retry, 'create', itemKey, createObj, createFn!, saveResult).then(
() => {
pendingCreates.delete(itemKey);
},
);
return retrySet(
params,
retry,
'create',
itemKey,
createObj,
changesById.get(itemKey)!,
createFn!,
saveResult,
).then(() => {
pendingCreates.delete(itemKey);
});
}),

// Handle updates
Expand All @@ -576,7 +592,16 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
}
const changed = (await transformOut(itemValue as TLocal, transform?.save)) as TRemote;
if (Object.keys(changed).length > 0) {
return retrySet(params, retry, 'update', itemKey, changed, updateFn!, saveResult);
return retrySet(
params,
retry,
'update',
itemKey,
changed,
changesById.get(itemKey)!,
updateFn!,
saveResult,
);
}
}),

Expand All @@ -587,9 +612,9 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
if (waitForSetParam) {
await waitForSet(waitForSetParam as any, changes, valuePrevious, { type: 'delete' });
}
const valueId = (valuePrevious as any)[fieldId];
const itemKey = (valuePrevious as any)[fieldId];

if (!valueId) {
if (!itemKey) {
console.error('[legend-state]: deleting item without an id');
return;
}
Expand All @@ -599,8 +624,9 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
params,
retry,
'delete',
valueId,
itemKey,
valuePrevious,
changesById.get(itemKey)!,
deleteFn!,
saveResult,
);
Expand All @@ -611,8 +637,9 @@ export function syncedCrud<TRemote extends object, TLocal = TRemote, TAsOption e
params,
retry,
'delete',
valueId,
{ [fieldId]: valueId, [fieldDeleted]: true } as any,
itemKey,
{ [fieldId]: itemKey, [fieldDeleted]: true } as any,
changesById.get(itemKey)!,
updateFn!,
saveResult,
);
Expand Down
27 changes: 16 additions & 11 deletions src/sync/syncObservable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ async function updateMetadataImmediate<T>(
// 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);
Expand All @@ -196,10 +196,10 @@ function updateMetadata<T>(
if (localState.timeoutSaveMetadata) {
clearTimeout(localState.timeoutSaveMetadata);
}
localState.timeoutSaveMetadata = setTimeout(
() => updateMetadataImmediate(value$, localState, syncState, syncOptions as SyncedOptions<T>, newMetadata),
0,
);
metadatas.set(value$, { ...(metadatas.get(value$) || {}), ...newMetadata });
localState.timeoutSaveMetadata = setTimeout(() => {
updateMetadataImmediate(value$, localState, syncState, syncOptions as SyncedOptions<T>, metadatas.get(value$)!);
}, 0);
}

interface QueuedChange<T = any> {
Expand Down Expand Up @@ -660,10 +660,11 @@ async function doChangeRemote(changeInfo: PreppedChangeRemote | undefined) {
onError: onSetError,
update: (params: UpdateSetFnParams<any>) => {
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;
Expand All @@ -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 = {};
Expand All @@ -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) {
Expand Down
114 changes: 112 additions & 2 deletions tests/keel.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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<T> = Result<T, APIError>;

Expand Down Expand Up @@ -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<any> => {
return { error: { message: 'test' } };
},
update: async ({ where }): Promise<any> => {
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<any> => {
return { error: { message: 'test' } };
},
update: async ({ where }): Promise<any> => {
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,
});
});
});

0 comments on commit 21b0af5

Please sign in to comment.