refactor: move temp to keyedById in updateTemp. No longer need tempsByNewId

Author: hamiltoes

src/service-module/service-module.actions.ts

@@ -312,7 +312,6 @@ export default function makeServiceActions(service: Service<any>) {
     addOrUpdate({ state, commit }, item) {
       const { idField } = state
       const id = getId(item, idField)
-      const existingItem = state.keyedById[id]
 
       const isIdOk = id !== null && id !== undefined
 
@@ -323,22 +322,14 @@ export default function makeServiceActions(service: Service<any>) {
         item = new service.FeathersVuexModel(item, { commit: false })
       }
 
-      // If the item has a matching temp, update the temp and provide it as the new item.
-      const temp = state.tempsByNewId[id]
-      if (temp) {
-        commit('merge', { dest: temp, source: item })
-        commit('remove__isTemp', temp)
-      }
       if (isIdOk) {
-        if (existingItem && temp) {
-          commit('replaceItemWithTemp', { item, temp })
+        if (state.keyedById[id]) {
+          commit('updateItem', item)
         } else {
-          existingItem
-            ? commit('updateItem', temp || item)
-            : commit('addItem', temp || item)
+          commit('addItem', item)
         }
       }
-      return temp || item
+      return item
     }
   }
   /**

src/service-module/service-module.getters.ts

@@ -9,7 +9,6 @@ import { filterQuery, sorter, select } from '@feathersjs/adapter-commons'
 import { globalModels as models } from './global-models'
 import _get from 'lodash/get'
 import _omit from 'lodash/omit'
-import _unionBy from 'lodash/unionBy'
 
 const FILTERS = ['$sort', '$limit', '$skip', '$select']
 const OPERATORS = ['$in', '$nin', '$lt', '$lte', '$gt', '$gte', '$ne', '$or']
@@ -27,7 +26,7 @@ export default function makeServiceGetters() {
       // Set params.temps to true to include the tempsById records
       params.temps = params.hasOwnProperty('temps') ? params.temps : false
 
-      const { paramsForServer, whitelist, idField, tempIdField } = state
+      const { paramsForServer, whitelist, keyedById } = state
       const q = _omit(params.query || {}, paramsForServer)
       const customOperators = Object.keys(q).filter(
         k => k[0] === '$' && !defaultOps.includes(k)
@@ -37,14 +36,10 @@ export default function makeServiceGetters() {
       const { query, filters } = filterQuery(cleanQuery, {
         operators: additionalOperators.concat(whitelist)
       })
-      let values = _.values(state.keyedById)
+      let values = _.values(keyedById)
 
       if (params.temps) {
-        values = _unionBy(
-          values,
-          _.values(state.tempsById),
-          i => i[tempIdField] || i[idField]
-        )
+        values = values.concat(_.values(state.tempsById))
       }
 
       values = values.filter(sift(query))

src/service-module/service-module.mutations.ts

@@ -142,9 +142,13 @@ export default function makeServiceMutations() {
     // can migrate the temp to the keyedById state.
     updateTemp(state, { id, tempId }) {
       const temp = state.tempsById[tempId]
-      if (state.tempsById) {
+      if (temp) {
         temp[state.idField] = id
-        state.tempsByNewId[id] = temp
+        Vue.delete(temp, '__isTemp')
+        Vue.delete(state.tempsById, tempId)
+        // If an item already exists in the store from the `created` event firing
+        // it will be replaced here
+        Vue.set(state.keyedById, id, temp)
       }
 
       // Add _id to temp's clone as well if it exists
@@ -153,32 +157,7 @@ export default function makeServiceMutations() {
       if (tempClone) {
         tempClone[state.idField] = id
         Model.copiesById[id] = tempClone
-      }
-    },
-
-    /**
-     * Overwrites the item with matching id with the temp record.
-     * This is to preserve reactivity for temp records.
-     */
-    replaceItemWithTemp(state, { item, temp }) {
-      const id = item[state.idField]
-      if (state.keyedById[id]) {
-        state.keyedById[id] = temp
-        Vue.delete(state.keyedById[id], '__isTemp')
-      }
-    },
-
-    remove__isTemp({ modelName, serverAlias, tempIdField }, temp) {
-      Vue.delete(temp, '__isTemp')
-
-      // Remove from temp's clone as well if it exists
-      const tempId = temp[tempIdField]
-      if (tempId) {
-        const Model = _get(models, `[${serverAlias}][${modelName}]`)
-        const tempClone = Model && Model.copiesById && Model.copiesById[tempId]
-        if (tempClone) {
-          Vue.delete(tempClone, '__isTemp')
-        }
+        Vue.delete(tempClone, '__isTemp')
       }
     },
 
@@ -194,24 +173,18 @@ export default function makeServiceMutations() {
       }
     },
 
-    // Removes temp records. Also cleans up tempsByNewId
+    // Removes temp records
     removeTemps(state, tempIds) {
-      const ids = tempIds.reduce((ids, id) => {
+      tempIds.forEach(id => {
         const temp = state.tempsById[id]
         if (temp) {
           if (temp[state.idField]) {
             // Removes __isTemp if created
             delete temp.__isTemp
             Vue.delete(temp, '__isTemp')
-            ids.push(temp[state.idField])
-          } else {
-            // Removes uncreated temp
-            ids.push(temp[state.tempIdField])
           }
         }
-        return ids
-      }, [])
-      state.tempsByNewId = _omit(state.tempsByNewId, ids)
+      })
       state.tempsById = _omit(state.tempsById, tempIds)
     },
 

src/service-module/service-module.state.ts

@@ -27,7 +27,6 @@ export interface ServiceStateExclusiveDefaults {
 
   keyedById: {}
   tempsById: {}
-  tempsByNewId: {}
   copiesById: {}
   namespace?: string
   pagination?: {
@@ -57,7 +56,6 @@ export interface ServiceState {
   idField: string
   keyedById: {}
   tempsById: {}
-  tempsByNewId: {}
   copiesById: {}
   whitelist: string[]
   paramsForServer: string[]
@@ -98,7 +96,6 @@ export default function makeDefaultState(options: MakeServicePluginOptions) {
     keyedById: {},
     copiesById: {},
     tempsById: {}, // Really should be called tempsByTempId
-    tempsByNewId: {}, // temporary storage for temps while getting transferred from tempsById to keyedById
     pagination: {
       defaultLimit: null,
       defaultSkip: null

test/service-module/make-service-plugin.test.ts

@@ -87,7 +87,6 @@ describe('makeServicePlugin', function() {
       servicePath: 'todos',
       skipRequestIfExists: false,
       tempsById: {},
-      tempsByNewId: {},
       whitelist: []
     }
 

test/service-module/model-temp-ids.test.ts

@@ -13,6 +13,7 @@ import Vue from 'vue/dist/vue'
 import Vuex from 'vuex'
 import { makeStore } from '../test-utils'
 import ObjectID from 'bson-objectid'
+import fastCopy from 'fast-copy'
 
 interface RootState {
   transactions: ServiceState
@@ -211,8 +212,6 @@ describe('Models - Temp Ids', function() {
       assert(response._id === 1)
       assert(response.__id, 'the temp id is still intact')
       assert(!store.state.things.tempsById[response.__id])
-      //@ts-ignore
-      assert(!Object.keys(store.state.things.tempsByNewId).length)
       assert(response === thing, 'maintained the reference')
     })
   })
@@ -401,12 +400,43 @@ describe('Models - Temp Ids', function() {
     const clone = thing.clone()
     assert(clone.__isTemp, 'Clone also has __isTemp')
 
-    store.commit('things/remove__isTemp', thing)
+    store.commit('things/updateTemp', { id: 42, tempId: thing.__id })
 
     assert(!thing.hasOwnProperty('__isTemp'), '__isTemp was removed from thing')
     assert(!clone.hasOwnProperty('__isTemp'), '__isTemp was removed from clone')
   })
 
+  it('updateTemp assigns ID to temp and migrates it from tempsById to keyedById', function() {
+    const { makeServicePlugin, BaseModel } = feathersVuex(feathersClient, {
+      idField: '_id',
+      serverAlias: 'temp-ids'
+    })
+    class Thing extends BaseModel {
+      public static modelName = 'Thing'
+    }
+    const store = new Vuex.Store<RootState>({
+      plugins: [
+        makeServicePlugin({
+          Model: Thing,
+          service: feathersClient.service('things')
+        })
+      ]
+    })
+
+    const thing = new Thing()
+    assert(thing.__id, 'thing has tempId')
+    assert(!thing._id, 'thing does not have _id')
+    assert(store.state.things.tempsById[thing.__id], 'thing is in tempsById')
+
+    store.commit('things/updateTemp', { id: 42, tempId: thing.__id })
+    assert(thing._id === 42, 'thing got _id')
+    assert(store.state.things.keyedById[42] === thing, 'thing is in keyedById')
+    assert(
+      !store.state.things.tempsById[thing.__id],
+      'thing is no longer in tempsById'
+    )
+  })
+
   it('Clone gets _id after save (create only called once)', async function() {
     // Test ensures subsequent calls to clone.save() do not call create
     const { makeServicePlugin, BaseModel } = feathersVuex(feathersClient, {
@@ -504,6 +534,10 @@ describe('Models - Temp Ids', function() {
     feathersClient.service('foos').hooks({
       before: {
         create: [
+          context => {
+            delete context.data.__id
+            delete context.data.__isTemp
+          },
           context => {
             context.result = { _id: 24, ...context.data }
             return context
@@ -526,7 +560,7 @@ describe('Models - Temp Ids', function() {
       },
       watch: {
         things(items) {
-          watchEvents.push(items)
+          watchEvents.push(fastCopy(items))
         }
       }
     }).$mount()

test/service-module/service-module.test.ts

@@ -658,7 +658,6 @@ describe('Service Module', function() {
         servicePath: 'service-todos',
         tempIdField: '__id',
         tempsById: {},
-        tempsByNewId: {},
         pagination: {
           defaultLimit: null,
           defaultSkip: null