fix(getPooled): Fixes a problem where threshold options could wrongly match on diff array lengths

Author: Rendez

src/IntersectionObserver.js

@@ -3,7 +3,7 @@ import { findDOMNode } from 'react-dom';
 import PropTypes from 'prop-types';
 import invariant from 'invariant';
 import IntersectionObserverContainer from './IntersectionObserverContainer';
-import { isDOMTypeElement } from './utils';
+import { isDOMTypeElement, shallowCompareOptions } from './utils';
 
 /**
  * The Intersection Observer API callback that is called whenever one element,
@@ -22,15 +22,6 @@ export function callback(changes, observer) {
   });
 }
 
-export function shallowCompareOptions(next, prev) {
-  if (Array.isArray(next) && Array.isArray(prev)) {
-    if (next.length === prev.length) {
-      return next.some((_, index) => shallowCompareOptions(next[index], prev[index]));
-    }
-  }
-  return next !== prev;
-}
-
 const observerOptions = ['root', 'rootMargin', 'threshold'];
 const objectProto = Object.prototype;
 

src/IntersectionObserverContainer.js

@@ -1,24 +1,20 @@
-import { parseRootMargin } from './utils';
+import { parseRootMargin, shallowCompareOptions } from './utils';
 
 export function getPooled(options = {}) {
-  let matchCount = 0;
   const root = options.root || null;
   const rootMargin = parseRootMargin(options.rootMargin);
   const threshold = Array.isArray(options.threshold)
     ? options.threshold
     : [typeof options.threshold !== 'undefined' ? options.threshold : 0];
   // eslint-disable-next-line no-restricted-syntax
   for (const observer of storage.keys()) {
-    let thresholdMatches = threshold;
-    let thresholds = observer.thresholds;
-    if (threshold.length > observer.thresholds) {
-      thresholdMatches = observer.thresholds;
-      thresholds = threshold;
-    }
-    matchCount += thresholds.every(v => v === thresholdMatches[thresholdMatches.indexOf(v)]);
-    matchCount += root === observer.root;
-    matchCount += rootMargin === observer.rootMargin;
-    if (matchCount === 3) {
+    const unmatched = [
+      [root, observer.root],
+      [rootMargin, observer.rootMargin],
+      [threshold, observer.thresholds],
+    ].some(option => shallowCompareOptions(...option));
+
+    if (!unmatched) {
       return observer;
     }
   }

src/utils.js

@@ -21,3 +21,12 @@ export function parseRootMargin(rootMargin) {
 
   return margins.join(' ');
 }
+
+export function shallowCompareOptions(next, prev) {
+  if (Array.isArray(next) && Array.isArray(prev)) {
+    if (next.length === prev.length) {
+      return next.some((_, index) => shallowCompareOptions(next[index], prev[index]));
+    }
+  }
+  return next !== prev;
+}

test/IntersectionObserver.test.js

@@ -239,41 +239,6 @@ describe('update', () => {
   });
 });
 
-describe('shallowCompareOptions', () => {
-  const comparerFn = (nextProps, prevProps) =>
-    ['disabled', 'root', 'rootMargin', 'threshold'].some(option =>
-      shallowCompareOptions(nextProps[option], prevProps[option])
-    );
-
-  test('should return true if threshold array length is not the same', () => {
-    const nextProps = { threshold: [0.25, 0.5] };
-    const prevProps = { threshold: [0.25, 0.5, 0.75] };
-
-    expect(comparerFn(nextProps, prevProps)).toBeTruthy();
-  });
-
-  test('should return true if threshold array length is the same but not equal', () => {
-    const nextProps = { threshold: [0.25, 0.75, 0.5] };
-    const prevProps = { threshold: [0.25, 0.5, 0.75] };
-
-    expect(comparerFn(nextProps, prevProps)).toBeTruthy();
-  });
-
-  test('should return false if options are equal', () => {
-    const nextProps = { disabled: true, root: 1, rootMargin: 2, threshold: [0.25, 0.75, 0.5] };
-    const prevProps = { ...nextProps };
-
-    expect(comparerFn(nextProps, prevProps)).toBeFalsy();
-  });
-
-  test('should return true if options are different', () => {
-    const nextProps = { disabled: true, root: 1, rootMargin: 2, threshold: [0.25, 0.75, 0.5] };
-    const prevProps = { ...nextProps, threshold: 1 };
-
-    expect(comparerFn(nextProps, prevProps)).toBeTruthy();
-  });
-});
-
 describe('callback', () => {
   test('should call propType onChange for each of the changes', () => {
     const spy = jest.fn();

test/IntersectionObserverContainer.test.js

@@ -47,6 +47,11 @@ describe('#getPooled', () => {
     expect(getPooled({ threshold: 0.5 })).toBeNull();
   });
 
+  test('returns nothing given threshold did not match', () => {
+    expect(getPooled({ rootMargin: '-10% 0%', threshold: [0, 0.5, 1, 0.25] })).toBeNull();
+    expect(getPooled({ rootMargin: '-10% 0%', threshold: [1, 0.5, 0] })).toBeNull();
+  });
+
   test('throws if rootMargin cannot be parsed', () => {
     expect(() => getPooled({ rootMargin: '-10% 0', threshold: 0 })).toThrow(
       'rootMargin must be specified in pixels or percent'

test/utils.test.js

@@ -1,5 +1,5 @@
 /* eslint-env jest */
-import { parseRootMargin } from '../src/utils';
+import { parseRootMargin, shallowCompareOptions } from '../src/utils';
 
 describe('parseRootMargin', () => {
   test('throws when using wrong units', () => {
@@ -13,3 +13,38 @@ describe('parseRootMargin', () => {
     expect(parseRootMargin('10px 5px 0%')).toBe('10px 5px 0% 5px');
   });
 });
+
+describe('shallowCompareOptions', () => {
+  const comparerFn = (nextProps, prevProps) =>
+    ['disabled', 'root', 'rootMargin', 'threshold'].some(option =>
+      shallowCompareOptions(nextProps[option], prevProps[option])
+    );
+
+  test('should return true if threshold array length is not the same', () => {
+    const nextProps = { threshold: [0.25, 0.5] };
+    const prevProps = { threshold: [0.25, 0.5, 0.75] };
+
+    expect(comparerFn(nextProps, prevProps)).toBeTruthy();
+  });
+
+  test('should return true if threshold array length is the same but not equal', () => {
+    const nextProps = { threshold: [0.25, 0.75, 0.5] };
+    const prevProps = { threshold: [0.25, 0.5, 0.75] };
+
+    expect(comparerFn(nextProps, prevProps)).toBeTruthy();
+  });
+
+  test('should return false if options are equal', () => {
+    const nextProps = { disabled: true, root: 1, rootMargin: 2, threshold: [0.25, 0.75, 0.5] };
+    const prevProps = { ...nextProps };
+
+    expect(comparerFn(nextProps, prevProps)).toBeFalsy();
+  });
+
+  test('should return true if options are different', () => {
+    const nextProps = { disabled: true, root: 1, rootMargin: 2, threshold: [0.25, 0.75, 0.5] };
+    const prevProps = { ...nextProps, threshold: 1 };
+
+    expect(comparerFn(nextProps, prevProps)).toBeTruthy();
+  });
+});