feat(query): consolidate oneListenerPerPath and allowMultipleListener…

…s logic - @alexmattson

The logic for `oneListenerPerPath: true` and `allowMultipleListeners: false` greatly overlapped. This PR removes the `oneListenerPerPath` config option and instead opts to standardize that functionality as the default. Now whenever you call the setListeners/unsetListeners it will track the number of listeners set up. Also gives the opt out with `allowMultipleListeners: true`

Changes:

1) Updated `setListeners` and `unsetListeners` functions 
2) Changed `oneListenerPerPath` tests to `allowMultipleListeners` tests
3) removed `oneListenerPerPath` from docs


### Check List
If not relevant to pull request, check off as complete

- [x] All tests passing
- [x] Docs updated with any changes or examples if applicable
- [x] Added tests to ensure new feature(s) work properly

### Relevant Issues
<!-- * #1 -->

README.md

@@ -251,7 +251,7 @@ After setting a listener/multiple listeners, you can unset them with the followi
 ```js
 store.firestore.unsetListener({ collection: 'cities' }),
 // of for any number of listeners at once :
-store.firestore.unsetListeners([query1Options, query2Options]), 
+store.firestore.unsetListeners([query1Options, query2Options]),
 // here query1Options as in { collection: 'cities' } for example
 ```
 
@@ -495,11 +495,6 @@ Default: `false`
 
 Whether or not to allow multiple listeners to be attached for the same query. If a function is passed the arguments it receives are `listenerToAttach`, `currentListeners`, and the function should return a boolean.
 
-#### oneListenerPerPath
-Default: `false`
-
-If set to true redux-firestore will attach a listener on the same path just once & will count how many the listener was set. When you try to unset the listener, it won't unset until you have less than 1 listeners on this path
-
 #### preserveOnDelete
 Default: `null`
 

src/actions/firestore.js

@@ -4,7 +4,6 @@ import { actionTypes } from '../constants';
 import {
   attachListener,
   detachListener,
-  listenerExists,
   orderedFromSnap,
   dataByIdSnapshot,
   getQueryConfig,
@@ -346,33 +345,19 @@ export function setListeners(firebase, dispatch, listeners) {
   }
 
   const { config } = firebase._;
-
-  // Only attach one listener (count of matching listener path calls is tracked)
-  if (config.oneListenerPerPath) {
-    return listeners.forEach(listener => {
-      const path = getQueryName(listener);
-      const oldListenerCount = pathListenerCounts[path] || 0;
-      pathListenerCounts[path] = oldListenerCount + 1;
-
-      // If we already have an attached listener exit here
-      if (oldListenerCount > 0) {
-        return;
-      }
-
-      setListener(firebase, dispatch, listener);
-    });
-  }
+  const { allowMultipleListeners } = config;
 
   return listeners.forEach(listener => {
-    // Config for supporting attaching of multiple listener callbacks
-    const multipleListenersEnabled = isFunction(config.allowMultipleListeners)
-      ? config.allowMultipleListeners(listener, firebase._.listeners)
-      : config.allowMultipleListeners;
+    const path = getQueryName(listener);
+    const oldListenerCount = pathListenerCounts[path] || 0;
+    const multipleListenersEnabled = isFunction(allowMultipleListeners)
+      ? allowMultipleListeners(listener, firebase._.listeners)
+      : allowMultipleListeners;
+
+    pathListenerCounts[path] = oldListenerCount + 1;
 
-    // Only attach listener if it does not already exist or
-    // if multiple listeners config is true or is a function which returns
-    // truthy value
-    if (!listenerExists(firebase, listener) || multipleListenersEnabled) {
+    // If we already have an attached listener exit here
+    if (oldListenerCount === 0 || multipleListenersEnabled) {
       setListener(firebase, dispatch, listener);
     }
   });
@@ -406,25 +391,23 @@ export function unsetListeners(firebase, dispatch, listeners) {
     );
   }
   const { config } = firebase._;
+  const { allowMultipleListeners } = config;
 
   // Keep one listener path even when detaching
-  if (config.oneListenerPerPath) {
-    listeners.forEach(listener => {
-      const path = getQueryName(listener);
-      pathListenerCounts[path] -= 1;
+  listeners.forEach(listener => {
+    const path = getQueryName(listener);
+    const listenerExists = pathListenerCounts[path] >= 1;
+    const multipleListenersEnabled = isFunction(allowMultipleListeners)
+      ? allowMultipleListeners(listener, firebase._.listeners)
+      : allowMultipleListeners;
 
+    if (listenerExists) {
+      pathListenerCounts[path] -= 1;
       // If we aren't supposed to have listners for this path, then remove them
-      if (pathListenerCounts[path] === 0) {
+      if (pathListenerCounts[path] === 0 || multipleListenersEnabled) {
         unsetListener(firebase, dispatch, listener);
       }
-    });
-
-    return;
-  }
-
-  listeners.forEach(listener => {
-    // Remove listener only if it exists
-    unsetListener(firebase, dispatch, listener);
+    }
   });
 }
 

src/constants.js

@@ -100,7 +100,7 @@ export const actionTypes = {
  * preserve from state when LISTENER_ERROR action is dispatched.
  * @property {Boolean} enhancerNamespace - `'firestore'` Namespace under which
  * enhancer places internal instance on redux store (i.e. store.firestore).
- * @property {Boolean|Function} allowMultipleListeners - `null` Whether or not
+ * @property {Boolean|Function} allowMultipleListeners - `false` Whether or not
  * to allow multiple listeners to be attached for the same query. If a function
  * is passed the arguments it receives are `listenerToAttach`,
  * `currentListeners`, and the function should return a boolean.

test/unit/actions/firestore.spec.js

@@ -654,12 +654,12 @@ describe('firestoreActions', () => {
         );
       });
 
-      describe('oneListenerPerPath', () => {
+      describe('allowMultipleListeners', () => {
         it('works with one listener', async () => {
           const fakeFirebaseWithOneListener = {
             _: {
               listeners: {},
-              config: { ...defaultConfig, oneListenerPerPath: true },
+              config: { ...defaultConfig, allowMultipleListeners: false },
             },
             firestore: () => ({
               collection: collectionClass,
@@ -688,7 +688,7 @@ describe('firestoreActions', () => {
           const fakeFirebaseWithOneListener = {
             _: {
               listeners: {},
-              config: { ...defaultConfig, oneListenerPerPath: true },
+              config: { ...defaultConfig, allowMultipleListeners: false },
             },
             firestore: () => ({
               collection: collectionClass,
@@ -769,12 +769,12 @@ describe('firestoreActions', () => {
         });
       });
 
-      describe('oneListenerPerPath option enabled', () => {
+      describe('allowMultipleListeners option enabled', () => {
         it('dispatches UNSET_LISTENER action', async () => {
           const fakeFirebaseWithOneListener = {
             _: {
               listeners: {},
-              config: { ...defaultConfig, oneListenerPerPath: true },
+              config: { ...defaultConfig, allowMultipleListeners: false },
             },
             firestore: () => ({
               collection: collectionClass,
@@ -793,7 +793,7 @@ describe('firestoreActions', () => {
           const fakeFirebaseWithOneListener = {
             _: {
               listeners: {},
-              config: { ...defaultConfig, oneListenerPerPath: true },
+              config: { ...defaultConfig, allowMultipleListeners: false },
             },
             firestore: () => ({
               collection: collectionClass,