Remove unnecessary branches (redux-orm#316)

* only check if parent is a model selector as otherwise it currently has to be a field selector

* set minimum branch coverage to 98%

Author: haveyaseen

jest.config.js

@@ -17,9 +17,9 @@ module.exports = {
     coveragePathIgnorePatterns: ["test/*"],
     coverageThreshold: {
         global: {
-            branches: 95,
+            branches: 98,
             functions: 100,
-            lines: 99,
+            lines: 100,
             statements: 99,
         },
     },

src/Session.js

@@ -176,21 +176,21 @@ const Session = class Session {
             if (!clauseFiltersByAttribute(clause, idAttribute)) {
                 return false;
             }
-            const id = clause.payload[idAttribute];
-            if (id === null) return false;
             /**
              * We previously knew which row we wanted to access,
              * so there was no need to scan the entire table.
              */
-            accessedIds.add(id);
+            accessedIds.add(clause.payload[idAttribute]);
             return true;
         });
 
         const accessedIndexes = [];
         const { indexes } = this.state[table];
         clauses.forEach(clause => {
             Object.keys(indexes).forEach(attr => {
-                if (!clauseFiltersByAttribute(clause, attr)) return;
+                if (!clauseFiltersByAttribute(clause, attr)) {
+                    return;
+                }
                 const value = clause.payload[attr];
                 accessedIndexes.push([table, attr, value]);
             });

src/db/Table.js

@@ -484,9 +484,7 @@ export class Table {
             indexIdsToDelete.forEach(([attr, value, id]) => {
                 const arr = nextIndexes[attr][value];
                 const idx = arr.indexOf(id);
-                if (idx !== -1) {
-                    ops.mutable.splice(idx, 1, [], arr);
-                }
+                ops.mutable.splice(idx, 1, [], arr);
             });
             indexIdsToAdd.forEach(([attr, value, id]) => {
                 ops.mutable.push(id, nextIndexes[attr][value]);
@@ -526,7 +524,7 @@ export class Table {
                                     [value]: ops.batch.filter(
                                         batchToken,
                                         rowId => rowId !== id,
-                                        indexMap[attr][value] || []
+                                        indexMap[attr][value]
                                     ),
                                 },
                                 indexMap[attr]
@@ -567,10 +565,7 @@ export class Table {
         if (withMutations) {
             idsToDelete.forEach(id => {
                 const idx = arr.indexOf(id);
-                if (idx !== -1) {
-                    ops.mutable.splice(idx, 1, [], arr);
-                }
-
+                ops.mutable.splice(idx, 1, [], arr);
                 ops.mutable.omit(id, branch[mapName]);
             });
             // delete ids from all indexes

src/fields/Field.js

@@ -5,10 +5,6 @@ import DefaultFieldInstaller from "./DefaultFieldInstaller";
  * @memberof module:fields
  */
 export class Field {
-    constructor() {
-        this.index = false;
-    }
-
     get installerClass() {
         return DefaultFieldInstaller;
     }
@@ -25,10 +21,6 @@ export class Field {
         return null;
     }
 
-    get installsForwardsDescriptor() {
-        return true;
-    }
-
     get installsForwardsVirtualField() {
         return false;
     }
@@ -40,6 +32,10 @@ export class Field {
     get installsBackwardsVirtualField() {
         return false;
     }
+
+    get index() {
+        return false;
+    }
 }
 
 export default Field;

src/fields/FieldInstallerTemplate.js

@@ -55,9 +55,7 @@ export class FieldInstallerTemplate {
     }
 
     run() {
-        if (this.field.installsForwardsDescriptor) {
-            this.installForwardsDescriptor();
-        }
+        this.installForwardsDescriptor();
         if (this.field.installsForwardsVirtualField) {
             this.installForwardsVirtualField();
         }

src/fields/ForeignKey.js

@@ -9,18 +9,17 @@ import {
  * @memberof module:fields
  */
 export class ForeignKey extends RelationalField {
-    constructor(...args) {
-        super(...args);
-        this.index = true;
-    }
-
     createForwardsDescriptor(fieldName, model, toModel, throughModel) {
         return forwardsManyToOneDescriptor(fieldName, toModel.modelName);
     }
 
     createBackwardsDescriptor(fieldName, model, toModel, throughModel) {
         return backwardsManyToOneDescriptor(fieldName, model.modelName);
     }
+
+    get index() {
+        return true;
+    }
 }
 
 export default ForeignKey;

src/redux.js

@@ -97,41 +97,36 @@ function toSelector(arg) {
     }
     if (arg instanceof SelectorSpec) {
         const { _orm: orm, cachePath } = arg;
-        let ormSelectors;
         let level;
-        if (cachePath && cachePath.length) {
-            // the selector cache for the spec's ORM
-            if (!selectorCache.has(orm)) {
-                selectorCache.set(orm, new Map());
-            }
-            ormSelectors = selectorCache.get(orm);
 
-            /**
-             * Drill down into selector map by cachePath.
-             *
-             * The selector itself is stored under a special SELECTOR_KEY
-             * so that we can store selectors below it as well.
-             * @private
-             */
-            level = ormSelectors;
-            for (let i = 0; i < cachePath.length; ++i) {
-                if (!level.has(cachePath[i])) {
-                    level.set(cachePath[i], new Map());
-                }
-                level = level.get(cachePath[i]);
-            }
-            if (level && level.has(SELECTOR_KEY)) {
-                // Cache hit: the selector has been created before
-                return level.get(SELECTOR_KEY);
+        // the selector cache for the spec's ORM
+        if (!selectorCache.has(orm)) {
+            selectorCache.set(orm, new Map());
+        }
+        const ormSelectors = selectorCache.get(orm);
+
+        /**
+         * Drill down into selector map by cachePath.
+         *
+         * The selector itself is stored under a special SELECTOR_KEY
+         * so that we can store selectors below it as well.
+         */
+        level = ormSelectors;
+        for (let i = 0; i < cachePath.length; ++i) {
+            if (!level.has(cachePath[i])) {
+                level.set(cachePath[i], new Map());
             }
+            level = level.get(cachePath[i]);
+        }
+        if (level && level.has(SELECTOR_KEY)) {
+            // Cache hit: the selector has been created before
+            return level.get(SELECTOR_KEY);
         }
 
         const selector = createSelectorFromSpec(arg);
 
-        if (cachePath && cachePath.length) {
-            // Save the selector at the cachePath position
-            level.set(SELECTOR_KEY, selector);
-        }
+        // Save the selector at the cachePath position
+        level.set(SELECTOR_KEY, selector);
 
         return selector;
     }

src/test/functional/immutableSessions.js

@@ -138,6 +138,11 @@ describe("Immutable session", () => {
         );
     });
 
+    it("withId(null) returns null", () => {
+        const { Book } = session;
+        expect(Book.withId(null)).toBe(null);
+    });
+
     it("withId returns null if model instance not found", () => {
         const { Book } = session;
         expect(Book.withId(9043290)).toBe(null);

src/utils.js

@@ -111,13 +111,12 @@ function attachQuerySetMethods(modelClass, querySetClass) {
                 if (typeof descriptor.get !== "undefined") {
                     descriptor.get = querySetGetterDelegatorFactory(methodName);
                     Object.defineProperty(modelClass, methodName, descriptor);
-                    defined = true;
-                } else if (typeof descriptor.value === "function") {
+                } else {
                     modelClass[methodName] = querySetDelegatorFactory(
                         methodName
                     );
-                    defined = true;
                 }
+                defined = true;
             }
             if (defined) {
                 leftToDefine.splice(i--, 1);