diff --git a/.changeset/lucky-poems-notice.md b/.changeset/lucky-poems-notice.md new file mode 100644 index 000000000..1fad09bf4 --- /dev/null +++ b/.changeset/lucky-poems-notice.md @@ -0,0 +1,5 @@ +--- +'houdini': patch +--- + +Fixed edge cases involving adding, removing, and deleting records back to back from in-memory cache" diff --git a/src/runtime/cache/cache.ts b/src/runtime/cache/cache.ts index 0fd7d6458..31aea0a69 100644 --- a/src/runtime/cache/cache.ts +++ b/src/runtime/cache/cache.ts @@ -919,7 +919,7 @@ class CacheInternal { innerType = typename as string } - // build up an + // if this isn't an embedded reference, use the entry's id in the link list if (!embedded) { const id = this.id(innerType, entry as {}) if (id) { @@ -955,7 +955,7 @@ class CacheInternal { // if there is only one layer in the cache, clean up the data if (this.storage.layerCount === 1) { - this.storage.topLayer.applyDeletes() + this.storage.topLayer.removeUndefinedFields() } } } diff --git a/src/runtime/cache/lists.ts b/src/runtime/cache/lists.ts index ebf946102..1a59442ce 100644 --- a/src/runtime/cache/lists.ts +++ b/src/runtime/cache/lists.ts @@ -253,7 +253,7 @@ export class List { } // if the id is not contained in the list, dont notify anyone - const value = this.cache._internal_unstable.storage.get(parentID, targetKey) + let value = this.cache._internal_unstable.storage.get(parentID, targetKey) .value as LinkedList if (!value || !value.includes(targetID)) { return @@ -275,6 +275,30 @@ export class List { // remove the target from the parent this.cache._internal_unstable.storage.remove(parentID, targetKey, targetID) + // if we are removing an id from a connection then the id we were given points to an edge + if (this.connection) { + // grab the index we are removing + const [, field, index] = targetID.match(/(.*)\[(\d+)\]$/) || [] + if (!field || !index) { + throw new Error('Could not find id of edge') + } + const newIDs = [] + + // every element in the linked list after the element we removed needs to have an updated id + for (let i = parseInt(index) + 1; i < value.length; i++) { + const to = `${field}[${i - 1}]` + newIDs.push(to) + this.cache._internal_unstable.storage.replaceID({ + from: `${field}[${i}]`, + to, + }) + } + + value = value.slice(0, parseInt(index)).concat(newIDs) + + this.cache._internal_unstable.storage.writeLink(parentID, targetKey, value) + } + // notify the subscribers about the change for (const spec of subscribers) { // trigger the update @@ -287,11 +311,6 @@ export class List { ) } - // if we are removing from a connection, delete the embedded edge holding the record - if (this.connection) { - this.cache._internal_unstable.storage.delete(targetID) - } - // return true if we deleted something return true } diff --git a/src/runtime/cache/storage.ts b/src/runtime/cache/storage.ts index 44956b3ce..0f632c413 100644 --- a/src/runtime/cache/storage.ts +++ b/src/runtime/cache/storage.ts @@ -58,6 +58,12 @@ export class InMemoryStorage { throw new Error('Could not find layer with id: ' + id) } + replaceID(replacement: { from: string; to: string }) { + for (const layer of this.data) { + layer.replaceID(replacement) + } + } + get( id: string, field: string @@ -83,7 +89,13 @@ export class InMemoryStorage { const layer = this.data[i] const [layerValue, kind] = layer.get(id, field) const layerOperations = layer.getOperations(id, field) || [] - layer.deletedIDs.forEach((v) => operations.remove.add(v)) + layer.deletedIDs.forEach((v) => { + // if the layer wants to undo a delete for the id + if (layer.operations[v]?.undoDeletesInList?.includes(field)) { + return + } + operations.remove.add(v) + }) // if the layer does not contain a value for the field, move on if (typeof layerValue === 'undefined' && layerOperations.length === 0) { @@ -303,11 +315,13 @@ export class Layer { const fieldOperations = this.operations[id]?.fields[field] // if the operation was globally deleted - if (this.operations[value]?.deleted) { + if (this.operations[value]?.deleted || this.deletedIDs.has(value)) { // undo the delete - // NOTE: this will bring it back to the all lists just because we inserted it - // to one containing the link - delete this.operations[value].deleted + this.operations[value] = { + ...this.operations[value], + undoDeletesInList: [...(this.operations[id]?.undoDeletesInList || []), field], + } + // the value could have been removed specifically from the list } else if (value && fieldOperations?.length > 0) { // if we have a field operation to remove the list, undo the operation @@ -343,7 +357,17 @@ export class Layer { this.deletedIDs = new Set() } - applyDeletes() { + replaceID({ from, to }: { from: string; to: string }) { + // any fields that existing in from, assign to to + this.fields[to] = this.fields[from] + this.links[to] = this.links[from] + this.operations[to] = this.operations[from] || { fields: {} } + if (this.deletedIDs.has(from)) { + this.deletedIDs.add(to) + } + } + + removeUndefinedFields() { // any field that's marked as undefined needs to be deleted for (const [id, fields] of Object.entries(this.fields)) { for (const [field, value] of Object.entries(fields)) { @@ -376,6 +400,8 @@ export class Layer { [id]: { ...this.operations[id], deleted: true, + // reapply any delete undos + undoDeletesInList: [], }, } @@ -429,7 +455,7 @@ export class Layer { } // if we are applying - if (ops.deleted) { + if (ops?.deleted) { delete this.fields[id] delete this.links[id] } @@ -437,6 +463,9 @@ export class Layer { // copy the field values for (const [id, values] of Object.entries(layer.fields)) { + if (!values) { + continue + } // we do have a record matching this id, copy the individual fields for (const [field, value] of Object.entries(values)) { this.writeField(id, field, value) @@ -445,6 +474,9 @@ export class Layer { // copy the link values for (const [id, values] of Object.entries(layer.links)) { + if (!values) { + continue + } // we do have a record matching this id, copy the individual links for (const [field, value] of Object.entries(values)) { this.writeLink(id, field, value) @@ -479,6 +511,7 @@ type LinkMap = EntityMap type OperationMap = { [id: string]: { deleted?: boolean + undoDeletesInList?: string[] fields: { [field: string]: ListOperation[] } } } diff --git a/src/runtime/cache/tests/list.test.ts b/src/runtime/cache/tests/list.test.ts index d9a0ab10a..40e67fbef 100644 --- a/src/runtime/cache/tests/list.test.ts +++ b/src/runtime/cache/tests/list.test.ts @@ -463,10 +463,146 @@ test('remove from connection', function () { expect(cache._internal_unstable.subscriptions.get('User:2', 'firstName')).toHaveLength(0) // but we're still subscribing to user 3 expect(cache._internal_unstable.subscriptions.get('User:3', 'firstName')).toHaveLength(1) - // make sure we marked the corresponding edge for deletion - expect( - cache._internal_unstable.storage.topLayer.operations['User:1.friends.edges[0]'].deleted - ).toBeTruthy() +}) + +test('element removed from list can be added back', function () { + // instantiate a cache + const cache = new Cache(config) + + const selection: SubscriptionSelection = { + viewer: { + type: 'User', + keyRaw: 'viewer', + fields: { + id: { + type: 'ID', + keyRaw: 'id', + }, + friends: { + type: 'User', + keyRaw: 'friends', + list: { + name: 'All_Users', + connection: true, + type: 'User', + }, + fields: { + edges: { + type: 'UserEdge', + keyRaw: 'edges', + fields: { + node: { + type: 'Node', + keyRaw: 'node', + abstract: true, + fields: { + __typename: { + type: 'String', + keyRaw: '__typename', + }, + id: { + type: 'ID', + keyRaw: 'id', + }, + firstName: { + type: 'String', + keyRaw: 'firstName', + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + // start off associated with one object + cache.write({ + selection, + data: { + viewer: { + id: '1', + friends: { + edges: [ + { + node: { + __typename: 'User', + id: '2', + firstName: 'jane2', + }, + }, + { + node: { + __typename: 'User', + id: '3', + firstName: 'jane', + }, + }, + ], + }, + }, + }, + }) + + // a function to spy on that will play the role of set + const set = jest.fn() + + // subscribe to the fields + cache.subscribe({ + rootType: 'Query', + set, + selection: selection, + }) + + // remove user 2 from the list + cache.list('All_Users').remove({ + id: '2', + }) + + cache.list('All_Users').append( + { + id: { + keyRaw: 'id', + type: 'String', + }, + firstName: { + keyRaw: 'firstName', + type: 'String', + }, + }, + { + __typename: 'User', + id: '2', + firstName: 'jane2', + }, + {} + ) + + expect(set).toHaveBeenNthCalledWith(2, { + viewer: { + id: '1', + friends: { + edges: [ + { + node: { + __typename: 'User', + id: '3', + firstName: 'jane', + }, + }, + { + node: { + __typename: 'User', + id: '2', + firstName: 'jane2', + }, + }, + ], + }, + }, + }) }) test('append in connection', function () { diff --git a/src/runtime/cache/tests/storage.test.ts b/src/runtime/cache/tests/storage.test.ts index 6690e04f4..840d24257 100644 --- a/src/runtime/cache/tests/storage.test.ts +++ b/src/runtime/cache/tests/storage.test.ts @@ -181,6 +181,30 @@ describe('in memory layers', function () { expect(storage.get('User:1', 'firstName').value).toBeUndefined() }) + test('can overwrite deletes for a specific link list', function () { + const storage = new InMemoryStorage() + + // add a base layer with some value + storage.writeLink('User:1', 'friends', ['User:2']) + + // add an optimistic layer that deletes the first entry + const layer = storage.createLayer(true) + layer.delete('User:2') + + // make sure its removed + expect(storage.get('User:1', 'friends').value).toEqual([]) + + // resolve the optimistic layer + storage.resolveLayer(layer.id) + // sanity check + expect(storage.get('User:1', 'friends').value).toEqual([]) + + // add the entry back to the list + storage.writeLink('User:1', 'friends', ['User:2']) + + expect(storage.get('User:1', 'friends').value).toEqual(['User:2']) + }) + test('deleting specific fields removes the field', function () { const storage = new InMemoryStorage() @@ -196,7 +220,7 @@ describe('in memory layers', function () { // delete the value storage.deleteField('User:1', 'firstName') - storage.topLayer.applyDeletes() + storage.topLayer.removeUndefinedFields() // look up the value now that it's been deleted expect(storage.get('User:1', 'firstName')).toEqual({ @@ -223,7 +247,7 @@ describe('in memory layers', function () { // delete the value storage.deleteField('User:1', 'firstName') - storage.topLayer.applyDeletes() + storage.topLayer.removeUndefinedFields() // look up the value now that it's been deleted expect(storage.get('User:1', 'firstName')).toEqual({