From 1b93810307d405417064114baf47b27630bc7ee0 Mon Sep 17 00:00:00 2001 From: Bobby Date: Mon, 9 Aug 2021 16:30:45 -0600 Subject: [PATCH 1/4] fix: mutate reads --- src/actions/firestore.js | 4 + src/createFirestoreInstance.js | 1 - src/reducers/cacheReducer.js | 30 ++- src/reducers/statusReducer.js | 8 +- test/unit/reducers/cacheReducer.spec.js | 256 ++++++++++++++++-------- test/unit/utils/query.spec.js | 5 +- 6 files changed, 199 insertions(+), 105 deletions(-) diff --git a/src/actions/firestore.js b/src/actions/firestore.js index 650f701a..4f122f35 100644 --- a/src/actions/firestore.js +++ b/src/actions/firestore.js @@ -416,9 +416,12 @@ export function runTransaction(firebase, dispatch, transactionPromise) { * @returns {Promise} Resolves with results of update call */ export function mutate(firebase, dispatch, writes) { + const timestamp = `${+new Date()}`; + return wrapInDispatch(dispatch, { ref: firebase, method: 'mutate', + meta: { timestamp }, args: [writes], types: [ { @@ -428,6 +431,7 @@ export function mutate(firebase, dispatch, writes) { actionTypes.MUTATE_SUCCESS, { type: actionTypes.MUTATE_FAILURE, + meta: { timestamp }, payload: { data: writes }, }, ], diff --git a/src/createFirestoreInstance.js b/src/createFirestoreInstance.js index 04d1c669..43f03215 100644 --- a/src/createFirestoreInstance.js +++ b/src/createFirestoreInstance.js @@ -102,7 +102,6 @@ export default function createFirestoreInstance(firebase, configs, dispatch) { * dispatch({ type: 'SOME_ACTION' }) * }) * }; - * */ export function getFirestore() { /* istanbul ignore next: Firestore instance always exists during tests */ diff --git a/src/reducers/cacheReducer.js b/src/reducers/cacheReducer.js index 9741d67f..f5a2a84d 100644 --- a/src/reducers/cacheReducer.js +++ b/src/reducers/cacheReducer.js @@ -164,6 +164,8 @@ const xfAllIds = ({ collection: path }) => /** * @name xfWhere + * @param getDoc.where + * @param getDoc * @param {Array.>} where - Firestore where clauses * @property {object.>} db * @property {object.>} dbo @@ -206,6 +208,8 @@ const xfWhere = ({ where }, getDoc) => { /** * @name xfOrder + * @param getDoc.orderBy + * @param getDoc * @param {Array.} order - Firestore order property * @property {object.>} db * @property {object.>} dbo @@ -234,11 +238,9 @@ const xfOrder = ({ orderBy: order }, getDoc) => { // TODO: refactor to manually lookup and compare const docs = tuples.map(([path, id]) => getDoc(path, id)); - const result = orderBy(docs, fields, direction).map( + return orderBy(docs, fields, direction).map( ({ id, path } = {}) => path && id && [path, id], ); - - return result; }); }; @@ -262,7 +264,8 @@ const xfLimit = ({ limit, endAt, endBefore }) => { * @param {?CacheState.database} db - * @param {?CacheState.databaseOverrides} dbo - * @param {RRFQuery} query - Firestore query - * @param {Boolean} isOptimisticWrite - includes optimistic data + * @param getDoc + * @param {boolean} isOptimisticWrite - includes optimistic data * @typedef {Function} xFormFilter - in optimistic reads and overrides * the reducer needs to take all documents and make a best effort to * filter down the document based on a cursor. @@ -329,6 +332,7 @@ const xfPaginate = (query, getDoc) => { * @name processOptimistic * Convert the query to a transducer for the query results * @param {?CacheState.database} database - + * @param state * @param {?CacheState.databaseOverrides} overrides - * @param {RRFQuery} query - query used to get data from firestore * @returns {Function} - Transducer will return a modifed array of documents @@ -340,9 +344,9 @@ function processOptimistic(query, state) { const dbo = databaseOverrides && databaseOverrides[collection]; const getDoc = (path, id) => { - if (path !== collection) console.log('-----', path, collection); const data = db[id] || {}; const override = dbo?.[id]; + return override ? { ...data, ...override } : data; }; @@ -530,7 +534,7 @@ function translateMutationToOverrides({ payload }, db = {}, dbo = {}) { const overrides = dbo[path] || {}; return { ...result, - [key]: { ...collection[id], ...(overrides[id] || {}) }, + [key]: { id, path, ...collection[id], ...(overrides[id] || {}) }, }; }, {}); } @@ -830,13 +834,21 @@ const mutation = (state, { action, key, path }) => { try { const result = produce(state, (draft) => { const done = mark(`cache.MUTATE_START`, key); + const { + meta: { timestamp }, + } = action; if (action.payload && action.payload.data) { const optimisiticUpdates = translateMutationToOverrides(action, draft.database) || []; - optimisiticUpdates.forEach(({ path: _path, id, ...data }) => { - info('overriding', `${_path}/${id}`, data); - setWith(draft, ['databaseOverrides', _path, id], data, Object); + optimisiticUpdates.forEach((data) => { + info('overriding', `${data.path}/${data.id}`, data); + setWith( + draft, + ['databaseOverrides', data.path, data.id], + data, + Object, + ); }); const updatePaths = [ diff --git a/src/reducers/statusReducer.js b/src/reducers/statusReducer.js index 1d538485..b51a1585 100644 --- a/src/reducers/statusReducer.js +++ b/src/reducers/statusReducer.js @@ -2,12 +2,8 @@ import { actionTypes } from '../constants'; import { getSlashStrPath, combineReducers } from '../utils/reducers'; import { getQueryName } from '../utils/query'; -const { - SET_LISTENER, - UNSET_LISTENER, - LISTENER_ERROR, - LISTENER_RESPONSE, -} = actionTypes; +const { SET_LISTENER, UNSET_LISTENER, LISTENER_ERROR, LISTENER_RESPONSE } = + actionTypes; /** * Reducer for requesting state.Changed by `START`, `NO_VALUE`, and `SET` actions. diff --git a/test/unit/reducers/cacheReducer.spec.js b/test/unit/reducers/cacheReducer.spec.js index 0af50d4e..d08987ff 100644 --- a/test/unit/reducers/cacheReducer.spec.js +++ b/test/unit/reducers/cacheReducer.spec.js @@ -3,15 +3,14 @@ import reducer from 'reducer'; import { actionTypes } from 'constants'; import { benchmark } from 'kelonio'; +import { debug } from 'debug'; import largeAction from './__stubs__/one_mb_action.json'; import appState from './__stubs__/app_state.json'; -import { debug } from 'debug'; // import * as mutate from 'utils/mutate'; const collection = 'testCollection'; const another = 'anotherCollection'; const path = `${collection}`; -const anotherPath = `${another}`; // --- data @@ -1356,16 +1355,6 @@ describe('cacheReducer', () => { 'obj.c.z': 10, }; - const expected = JSON.parse( - JSON.stringify({ - array: [1, 2, 3, 4, 5], - obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, - vanilla: 'some-data', - date: new Date('2021-01-01'), - // "serverTimestamp": new Date()} - }), - ); - // Initial seed const action1 = { meta: { @@ -1382,6 +1371,7 @@ describe('cacheReducer', () => { meta: { collection, doc: updates.id, + timestamp: '999', }, payload: { data: { ...updates }, @@ -1391,12 +1381,35 @@ describe('cacheReducer', () => { const pass1 = reducer(initialState, action1); const pass2 = reducer(pass1, action2); - expect(pass2.cache.testStoreAs.ordered).to.eql([[path, doc1.id]]); + expect(pass2.cache.testStoreAs).to.eql({ + ordered: [['testCollection', 'testDocId1']], + collection: 'testCollection', + where: ['key1', '==', 'value1'], + storeAs: 'testStoreAs', + via: 'cache', + }); expect(pass2.cache.database).to.eql({ - [collection]: { [doc1.id]: doc1 }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + key1: 'value1', + array: [1, 2, 3, 4], + obj: { a: 1, b: { x: 0 }, c: { z: 9 } }, + }, + }, }); expect(pass2.cache.databaseOverrides).to.eql({ - [collection]: { [doc1.id]: expected }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + vanilla: 'some-data', + date: '2021-01-01T00:00:00.000Z', + array: [1, 2, 3, 4, 5], + obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, + }, + }, }); }); @@ -1442,6 +1455,7 @@ describe('cacheReducer', () => { meta: { collection, doc: updates.id, + timestamp: '999', }, payload: { data: [{ ...updates }], @@ -1451,25 +1465,38 @@ describe('cacheReducer', () => { const pass1 = reducer(initialState, action1); const pass2 = reducer(pass1, action2); - expect(pass2.cache.testStoreAs.ordered).to.eql([[path, doc1.id]]); + expect(pass2.cache.testStoreAs).to.eql({ + ordered: [['testCollection', 'testDocId1']], + collection: 'testCollection', + where: ['key1', '==', 'value1'], + storeAs: 'testStoreAs', + via: 'cache', + }); expect(pass2.cache.database).to.eql({ - [collection]: { [doc1.id]: doc1 }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + key1: 'value1', + number: 11, + array: [1, 2, 3, 4], + obj: { a: 1, b: { x: 0 }, c: { z: 9 } }, + }, + }, }); - expect(pass2.cache.databaseOverrides).to.eql( - JSON.parse( - JSON.stringify({ - [collection]: { - [doc1.id]: { - array: [1, 3, 4], - number: 15, - obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, - date: new Date('2021-01-01'), - }, - }, - }), - ), - ); + expect(pass2.cache.databaseOverrides).to.eql({ + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + number: 15, + date: '2021-01-01T00:00:00.000Z', + array: [1, 3, 4], + obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, + }, + }, + }); }); it('Firestore transaction update adds override', () => { @@ -1514,6 +1541,7 @@ describe('cacheReducer', () => { meta: { collection, doc: updates.id, + timestamp: '999', }, payload: { data: { @@ -1538,27 +1566,40 @@ describe('cacheReducer', () => { const pass1 = reducer(initialState, action1); const pass2 = reducer(pass1, action2); - expect(pass2.cache.testStoreAs.ordered).to.eql([[path, doc1.id]]); + expect(pass2.cache.testStoreAs).to.eql({ + ordered: [['testCollection', 'testDocId1']], + collection: 'testCollection', + storeAs: 'testStoreAs', + via: 'cache', + }); + expect(pass2.cache.database).to.eql({ - [collection]: { [doc1.id]: doc1 }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + key1: 'value1', + number: 11, + multipled: 3, + array: [1, 2, 3, 4], + obj: { a: 1, b: { x: 0 }, c: { z: 9 } }, + }, + }, }); - expect(pass2.cache.databaseOverrides).to.eql( - JSON.parse( - JSON.stringify({ - [collection]: { - [doc1.id]: { - multipled: 12, - number: 15, - array: [1, 3, 4], - obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, - date: new Date('2021-01-01'), - // "serverTimestamp": new Date()} - }, - }, - }), - ), - ); + expect(pass2.cache.databaseOverrides).to.eql({ + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + multipled: 12, + number: 15, + date: '2021-01-01T00:00:00.000Z', + array: [1, 3, 4], + obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, + }, + }, + }); }); it('Firestore transaction update single write adds override', () => { @@ -1589,6 +1630,7 @@ describe('cacheReducer', () => { meta: { collection, storeAs: 'testStoreAs', + timestamp: '999', }, payload: { data: { [doc1.id]: doc1 }, @@ -1603,6 +1645,7 @@ describe('cacheReducer', () => { meta: { collection, doc: updates.id, + timestamp: '999', }, payload: { data: { @@ -1625,27 +1668,41 @@ describe('cacheReducer', () => { const pass1 = reducer(initialState, action1); const pass2 = reducer(pass1, action2); - expect(pass2.cache.testStoreAs.ordered).to.eql([[path, doc1.id]]); + expect(pass2.cache.testStoreAs).to.eql({ + ordered: [['testCollection', 'testDocId1']], + collection: 'testCollection', + storeAs: 'testStoreAs', + timestamp: '999', + via: 'cache', + }); + expect(pass2.cache.database).to.eql({ - [collection]: { [doc1.id]: doc1 }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + key1: 'value1', + number: 11, + multipled: 3, + array: [1, 2, 3, 4], + obj: { a: 1, b: { x: 0 }, c: { z: 9 } }, + }, + }, }); - expect(pass2.cache.databaseOverrides).to.eql( - JSON.parse( - JSON.stringify({ - [collection]: { - [doc1.id]: { - multipled: 12, - number: 15, - array: [1, 3, 4], - obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, - date: new Date('2021-01-01'), - // "serverTimestamp": new Date()} - }, - }, - }), - ), - ); + expect(pass2.cache.databaseOverrides).to.eql({ + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + multipled: 12, + number: 15, + date: '2021-01-01T00:00:00.000Z', + array: [1, 3, 4], + obj: { a: 0, b: { y: 9 }, c: { z: 10 } }, + }, + }, + }); }); it('Firestore does not support queries inside a transaction', () => { @@ -1678,6 +1735,7 @@ describe('cacheReducer', () => { meta: { collection, doc: updates.id, + timestamp: '999', }, payload: { data: { @@ -1718,21 +1776,6 @@ describe('cacheReducer', () => { vanilla: 'some-data', }; - const expected2 = JSON.parse( - JSON.stringify({ - ...doc1, - key1: 'value1', - ...updates, - }), - ); - - const expected3 = JSON.parse( - JSON.stringify({ - ...doc1, - key1: 'value1', - }), - ); - // Initial seed const action1 = { meta: { @@ -1754,6 +1797,7 @@ describe('cacheReducer', () => { meta: { collection, doc: updates.id, + timestamp: '999', }, payload: { data: { @@ -1789,18 +1833,58 @@ describe('cacheReducer', () => { expect(pass2.cache.database.testCollection.testDocId1).to.eql(doc1); expect(pass3.cache.database.testCollection.testDocId1).to.eql(doc1); + expect(pass2.cache.testStoreAs).to.eql({ + ordered: [['testCollection', 'testDocId1']], + collection: 'testCollection', + where: ['key1', '==', 'value1'], + storeAs: 'testStoreAs', + via: 'cache', + }); + expect(pass2.cache.database).to.eql({ - [collection]: { [doc1.id]: doc1 }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + key1: 'value1', + }, + }, }); + expect(pass2.cache.databaseOverrides).to.eql({ - [collection]: { [doc1.id]: { vanilla: 'some-data' } }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + vanilla: 'some-data', + }, + }, }); // overrides were deleted expect(pass3.cache.database).to.eql({ - [collection]: { [doc1.id]: doc1 }, + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + key1: 'value1', + }, + }, + }); + + expect(pass3.cache.databaseOverrides).to.eql({}); + + expect(pass3.cache.mutations).to.eql({ + 999: { + testCollection: { + testDocId1: { + path: 'testCollection', + id: 'testDocId1', + vanilla: 'some-data', + }, + }, + }, }); - expect(pass3.cache.databaseOverrides[collection]).to.eql(); }); }); describe('Speed test (on 2015 Dual-core 1.5Ghz i5 w/ 8GB 1600 DDR3)', () => { diff --git a/test/unit/utils/query.spec.js b/test/unit/utils/query.spec.js index 9b9a8345..d47f0b3b 100644 --- a/test/unit/utils/query.spec.js +++ b/test/unit/utils/query.spec.js @@ -612,9 +612,8 @@ describe('query utils', () => { describe('startAfter', () => { it('calls startAfter if valid', () => { - const { theFirebase, theSpy, theMeta } = fakeFirebaseWith( - 'startAfter', - ); + const { theFirebase, theSpy, theMeta } = + fakeFirebaseWith('startAfter'); result = firestoreRef(theFirebase, theMeta); expect(result).to.be.an('object'); expect(theSpy).to.be.calledWith(theMeta.subcollections[0].startAfter); From 89a70367f9dc4e6ca14883690ec17486d95b6eb4 Mon Sep 17 00:00:00 2001 From: Bobby Date: Mon, 9 Aug 2021 18:11:40 -0600 Subject: [PATCH 2/4] fix: pagination aligned with firestore results --- src/reducers/cacheReducer.js | 3 ++- test/unit/reducers/cacheReducer.spec.js | 32 ++++++++++++------------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/reducers/cacheReducer.js b/src/reducers/cacheReducer.js index f5a2a84d..d1d031c4 100644 --- a/src/reducers/cacheReducer.js +++ b/src/reducers/cacheReducer.js @@ -279,8 +279,9 @@ const xfPaginate = (query, getDoc) => { const end = endAt || endBefore; const isAfter = startAfter !== undefined; const isBefore = endBefore !== undefined; + const needsPagination = start || end || false; - if (!order || !isOptimisticRead || !!start || !!end) return identity; + if (!needsPagination || !order || !isOptimisticRead) return identity; const isFlat = typeof order[0] === 'string'; const orders = isFlat ? [order] : order; diff --git a/test/unit/reducers/cacheReducer.spec.js b/test/unit/reducers/cacheReducer.spec.js index d08987ff..fd653bf6 100644 --- a/test/unit/reducers/cacheReducer.spec.js +++ b/test/unit/reducers/cacheReducer.spec.js @@ -265,8 +265,8 @@ describe('cacheReducer', () => { const passA = reducer(primedState, stateDesc); expect(passA.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId4'], - ['testCollection', 'testDocId3'], + ['testCollection', 'testDocId1'], + ['testCollection', 'testDocId0'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -279,8 +279,8 @@ describe('cacheReducer', () => { const passB = reducer(primedState, stateAsc); expect(passB.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId0'], - ['testCollection', 'testDocId1'], + ['testCollection', 'testDocId3'], + ['testCollection', 'testDocId4'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -306,8 +306,8 @@ describe('cacheReducer', () => { const passA = reducer(primedState, stateDesc); expect(passA.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId4'], - ['testCollection', 'testDocId3'], + ['testCollection', 'testDocId1'], + ['testCollection', 'testDocId0'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -320,8 +320,8 @@ describe('cacheReducer', () => { const passB = reducer(primedState, stateAsc); expect(passB.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId0'], - ['testCollection', 'testDocId1'], + ['testCollection', 'testDocId3'], + ['testCollection', 'testDocId4'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -344,8 +344,8 @@ describe('cacheReducer', () => { const passA = reducer(primedState, stateDesc); expect(passA.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId1'], - ['testCollection', 'testDocId0'], + ['testCollection', 'testDocId4'], + ['testCollection', 'testDocId3'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -358,8 +358,8 @@ describe('cacheReducer', () => { const passB = reducer(primedState, stateAsc); expect(passB.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId3'], - ['testCollection', 'testDocId4'], + ['testCollection', 'testDocId0'], + ['testCollection', 'testDocId1'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -385,8 +385,8 @@ describe('cacheReducer', () => { const passA = reducer(primedState, stateDesc); expect(passA.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId1'], - ['testCollection', 'testDocId0'], + ['testCollection', 'testDocId4'], + ['testCollection', 'testDocId3'], ], collection: 'testCollection', storeAs: 'testStoreAs', @@ -399,8 +399,8 @@ describe('cacheReducer', () => { const passB = reducer(primedState, stateAsc); expect(passB.cache.testStoreAs).to.eql({ ordered: [ - ['testCollection', 'testDocId3'], - ['testCollection', 'testDocId4'], + ['testCollection', 'testDocId0'], + ['testCollection', 'testDocId1'], ], collection: 'testCollection', storeAs: 'testStoreAs', From c711ac76eeeeadd9b592380657d47e6ca16e4a35 Mon Sep 17 00:00:00 2001 From: Bobby Date: Mon, 9 Aug 2021 18:25:29 -0600 Subject: [PATCH 3/4] chore: bump version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 08a66b50..656973b8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "redux-firestore", - "version": "1.1.1", + "version": "1.1.2", "description": "Redux bindings for Firestore.", "main": "lib/index.js", "module": "es/index.js", From 30c3b43dac1e0fb2731361f9e79cfcb9bd805104 Mon Sep 17 00:00:00 2001 From: Bobby Date: Mon, 9 Aug 2021 18:32:31 -0600 Subject: [PATCH 4/4] chore: update test --- test/unit/reducers/cacheReducer.spec.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/unit/reducers/cacheReducer.spec.js b/test/unit/reducers/cacheReducer.spec.js index fd653bf6..79854175 100644 --- a/test/unit/reducers/cacheReducer.spec.js +++ b/test/unit/reducers/cacheReducer.spec.js @@ -529,6 +529,19 @@ describe('cacheReducer', () => { expect(pass1.cache.testStoreAs.ordered[0][1]).to.eql(doc1.id); expect(pass2.cache.testStoreAs.ordered[0][1]).to.eql(doc2.id); expect(pass3.cache.testStoreAs.ordered[0][1]).to.eql(doc1.id); + + // overrides must be cleared + expect(pass2.cache.databaseOverrides).to.eql({ + testCollection: { + testDocId1: { + id: 'testDocId1', + key1: 'value1', + key2: 'other', + path: 'testCollection', + }, + }, + }); + expect(pass3.cache.databaseOverrides).to.eql({}); }); it('overrides synchronously moves to new query', () => {