fix(performance): changes to improve performance

I believe there are two aspects that affected the performance of JNS since v1.6.3 - one was the mutations. This has been 'improved' by introducing a set object to make sure event listeners are not added to existing nodes. Another improvement was to refactor out the parentNode call before building the array of WarningHighlights.
defmethodinc · Feb 17, 2021 · bca3ac9 · bca3ac9
1 parent 3d1e32a
commit bca3ac9
Show file tree

Hide file tree

Showing 10 changed files with 181 additions and 272 deletions.
diff --git a/spec/ContentEditableDivSpec.test.js b/spec/ContentEditableDivSpec.test.js
@@ -1,16 +1,14 @@
-import { ifEmailModified } from '../src/callbacks/ContentEditableDiv.js';
+import { forEachUniqueContentEditable } from '../src/callbacks/ContentEditableDiv.js';
 
 const buildMutation = (type, target) => {
   return { type, target };
 };
 
 describe('handleContentEditableDivChange', () => {
-  const action = (node) => console.log(node);
-  let spy;
-  let handler = ifEmailModified(action);
-
+  let mockCallback, handler;
   beforeEach(() => {
-    spy = jest.spyOn(console, 'log').mockImplementationOnce(() => {});
+    mockCallback = jest.fn((mutation) => mutation);
+    handler = forEachUniqueContentEditable(mockCallback);
   });
   describe('when target is node with contentEditable attribute', () => {
     it('should not call the action if mutation is not type childList', () => {
@@ -20,17 +18,29 @@ describe('handleContentEditableDivChange', () => {
       const mutation = buildMutation('characterData', div);
       handler([mutation]);
 
-      expect(spy).not.toHaveBeenCalledWith(mutation);
+      expect(mockCallback.mock.calls.length).toBe(0);
+    });
+
+    it('should not call the action callback more than once for each unique mutation of type childList', () => {
+      const div = document.createElement('div');
+      div.setAttribute('contentEditable', 'true');
+      div.setAttribute('id', 'uniqueID');
+
+      const mutation = buildMutation('childList', div);
+      handler([mutation]);
+      handler([mutation]);
+      expect(mockCallback.mock.calls.length).toBe(1);
     });
 
-    it('should call the action callback for each mutation of type childList', () => {
+    it('should call the action callback once for each unique mutation of type childList', () => {
       const div = document.createElement('div');
       div.setAttribute('contentEditable', 'true');
 
       const mutation = buildMutation('childList', div);
       handler([mutation]);
 
-      expect(spy).toHaveBeenCalledWith(mutation);
+      expect(mockCallback.mock.calls.length).toBe(1);
+      expect(mockCallback.mock.calls[0][0]).toBe(mutation);
     });
   });
 
@@ -41,7 +51,7 @@ describe('handleContentEditableDivChange', () => {
       const mutation = buildMutation('childList', div);
       handler([mutation]);
 
-      expect(spy).not.toHaveBeenCalledWith(mutation);
+      expect(mockCallback.mock.calls.length).toBe(0);
     });
 
     it('should not call the action if mutation is not type childList', () => {
@@ -50,7 +60,7 @@ describe('handleContentEditableDivChange', () => {
       const mutation = buildMutation('characterData', div);
       handler([mutation]);
 
-      expect(spy).not.toHaveBeenCalledWith(mutation);
+      expect(mockCallback.mock.calls.length).toBe(0);
     });
   });
 });
diff --git a/spec/JustNotSorrySpec.test.js b/spec/JustNotSorrySpec.test.js
@@ -119,6 +119,8 @@ describe('JustNotSorry', () => {
         expect(handleSearch).toHaveBeenCalledTimes(1);
       });
     });
+
+    //TODO cut
   });
 
   describe('#resetState', () => {
@@ -146,114 +148,17 @@ describe('JustNotSorry', () => {
     });
   });
 
-  describe('#search', () => {
-    it('adds a valid range for a punctuation keyword', () => {
-      const node = generateEditableDiv({ id: 'meaningless-id' }, 'test!!!');
-      const domNode = node.getDOMNode();
-      const ranges = instance.search(
-        domNode,
-        buildWarning('\\b!{3,}\\B', 'warning message')
-      );
-      expect(ranges.length).toEqual(1);
-      expect(ranges[0].rangeToHighlight).toBeTruthy();
-      expect(ranges[0]).toEqual(
-        expect.objectContaining({
-          message: 'warning message',
-          parentNode: domNode.parentNode,
-        })
-      );
-    });
+  describe('#updateWarnings', () => {
+    it('updates state and parentNode for a match', () => {
+      const elem = generateEditableDiv({ id: 'meaningless-id' }, 'test!!!');
 
-    it('adds a warning for a single keyword', () => {
-      const node = generateEditableDiv(
-        { id: 'meaningless-id' },
-        'test just test'
-      );
+      const domNode = elem.getDOMNode();
+      instance.updateWarnings(domNode, [
+        buildWarning('\\b!{3,}\\B', 'warning message'),
+      ]);
 
-      const domNode = node.getDOMNode();
-      const ranges = instance.search(
-        domNode,
-        buildWarning('just', 'warning message')
-      );
-
-      expect(ranges.length).toEqual(1);
-      expect(ranges[0].rangeToHighlight).toBeTruthy();
-      expect(ranges[0]).toEqual(
-        expect.objectContaining({
-          message: 'warning message',
-          parentNode: domNode.parentNode,
-        })
-      );
-    });
-
-    it('does not add warnings for partial matches', () => {
-      const node = generateEditableDiv({ id: 'div-id' }, 'test justify test');
-
-      const domNode = node.getDOMNode();
-      const ranges = instance.search(
-        domNode,
-        buildWarning('\\b(just)\\b', 'warning message')
-      );
-      expect(ranges.length).toEqual(0);
-      expect(ranges).toEqual([]);
-    });
-
-    it('matches case insensitive', () => {
-      const node = generateEditableDiv({ id: 'div-case' }, 'jUsT kidding');
-
-      const domNode = node.getDOMNode();
-      const ranges = instance.search(
-        domNode,
-        buildWarning('just', 'warning message')
-      );
-
-      expect(ranges.length).toEqual(1);
-      expect(ranges[0].rangeToHighlight).toBeTruthy();
-      expect(ranges[0]).toEqual(
-        expect.objectContaining({
-          message: 'warning message',
-          parentNode: domNode.parentNode,
-        })
-      );
-    });
-
-    it('catches keywords with punctuation', () => {
-      const node = generateEditableDiv({ id: 'div-punctuation' }, 'just. test');
-
-      const domNode = node.getDOMNode();
-      const ranges = instance.search(
-        domNode,
-        buildWarning('just', 'warning message')
-      );
-      expect(ranges.length).toEqual(1);
-      expect(ranges[0].rangeToHighlight).toBeTruthy();
-      expect(ranges[0]).toEqual(
-        expect.objectContaining({
-          message: 'warning message',
-          parentNode: domNode.parentNode,
-        })
-      );
-    });
-
-    it('matches phrases', () => {
-      const node = generateEditableDiv(
-        { id: 'div-phrase' },
-        'my cat is so sorry because of you'
-      );
-
-      const domNode = node.getDOMNode();
-      const ranges = instance.search(
-        domNode,
-        buildWarning('so sorry', 'warning message')
-      );
-      expect(ranges.length).toEqual(1);
-      expect(ranges[0].rangeToHighlight).toBeTruthy();
-      expect(ranges[0]).toEqual(
-        expect.objectContaining({
-          message: 'warning message',
-          parentNode: domNode.parentNode,
-        })
-      );
+      expect(wrapper.state('warnings').length).toBe(1);
+      expect(wrapper.state('parentNode')).toBe(domNode.parentNode);
     });
 
     it('does not add warnings for tooltip matches', () => {
@@ -282,17 +187,4 @@ describe('JustNotSorry', () => {
       expect(wrapper.state('warnings')).toEqual([]);
     });
   });
-
-  describe('#searchEmail', () => {
-    it('adds warnings to all keywords', () => {
-      const node = generateEditableDiv(
-        { id: 'div-keywords' },
-        'I am just so sorry. Yes, just.'
-      );
-
-      const domNode = node.getDOMNode();
-      instance.searchEmail(domNode);
-      expect(wrapper.state('warnings').length).toEqual(3);
-    });
-  });
 });
diff --git a/spec/RangeFinderSpec.test.js b/spec/RangeFinderSpec.test.js
@@ -0,0 +1,27 @@
+import { findRanges } from '../src/helpers/RangeFinder';
+
+describe('RangeFinder', () => {
+  const PHRASE_TO_FIND = {
+    regex: new RegExp('textToFind', 'ig'),
+    message: 'text found!',
+  };
+  let element;
+  beforeEach(() => {
+    element = document.createElement('div');
+  });
+  it('should return empty array if given empty array', () => {
+    const ranges = findRanges(element, []);
+    expect(ranges.length).toBe(0);
+  });
+
+  it('should return array with message and valid range for each match found', () => {
+    element.textContent = 'textToFind';
+
+    const ranges = findRanges(element, [PHRASE_TO_FIND]);
+
+    expect(ranges.length).toBe(1);
+    expect(ranges).toEqual([
+      { message: 'text found!', rangeToHighlight: new Range() },
+    ]);
+  });
+});
diff --git a/spec/UtilSpec.test.js b/spec/UtilSpec.test.js
@@ -1,4 +1,4 @@
-import * as Util from '../src/components/util.js';
+import * as Util from '../src/helpers/util.js';
 
 describe('Util', () => {
   describe('#debounce', () => {

diff --git a/spec/WarningSpec.test.js b/spec/WarningSpec.test.js
@@ -1,5 +1,8 @@
 import { h } from 'preact';
-import Warning, { HighlightHelper } from '../src/components/Warning.js';
+import Warning, {
+  calculateCoords,
+  getHighlight,
+} from '../src/components/Warning.js';
 import { configure, shallow } from 'enzyme';
 import Adapter from 'enzyme-adapter-preact-pure';
 
@@ -55,7 +58,13 @@ describe('<Warning/>', () => {
   };
 
   beforeEach(() => {
-    wrapper = shallow(<Warning value={testProps.value} key={testProps.key} />);
+    wrapper = shallow(
+      <Warning
+        parentRect={parent.getBoundingClientRect()}
+        value={testProps.value}
+        key={testProps.key}
+      />
+    );
   });
 
   it('should return a warning div', () => {
@@ -66,10 +75,8 @@ describe('<Warning/>', () => {
 
 describe('#calculateCoords', () => {
   it('should return undefined if the parentNode or rangeToHighlight are invalid', () => {
-    expect(HighlightHelper.calculateCoords(null, null)).toEqual(undefined);
-    expect(HighlightHelper.calculateCoords(undefined, undefined)).toEqual(
-      undefined
-    );
+    expect(calculateCoords(null, null)).toEqual(undefined);
+    expect(calculateCoords(undefined, undefined)).toEqual(undefined);
   });
 
   it('should return valid coords when both parentNode and rangeToHighlight are valid', () => {
@@ -78,55 +85,18 @@ describe('#calculateCoords', () => {
     expect(rectsToHighlight.length).toEqual(1);
 
     const rect = rectsToHighlight[0];
-    const coords = HighlightHelper.calculateCoords(
-      parentNode.getBoundingClientRect(),
-      rect
-    );
+    const coords = calculateCoords(parentNode.getBoundingClientRect(), rect);
     expect(coords).toEqual({
       top: 65,
       left: 0,
     });
   });
 });
 
-describe('#getHighlights', () => {
-  it('should return undefined if the parentNode or rangeToHighlight are invalid', () => {
-    expect(HighlightHelper.getHighlights(null, null)).toEqual(undefined);
-    expect(HighlightHelper.getHighlights(undefined, undefined)).toEqual(
-      undefined
-    );
-  });
-
-  it('should return valid highlights when both parentNode and rangeToHighlight are valid', () => {
-    const parentNode = parent;
-    const rangeToHighlight = range;
-
-    const highlights = HighlightHelper.getHighlights(
-      parentNode,
-      rangeToHighlight
-    );
-    expect(highlights.length).toEqual(1);
-    expect(highlights[0]).toEqual({
-      style: {
-        top: '62px',
-        left: '0px',
-        width: '39px',
-        height: '3px',
-        zIndex: 10,
-        position: 'absolute',
-        padding: '0px',
-      },
-      position: 'bottom',
-    });
-  });
-});
-
 describe('#setNodeStyles', () => {
   it('should return undefined if the parentNode or rangeToHighlight are invalid', () => {
-    expect(HighlightHelper.getHighlight(null, null)).toEqual(undefined);
-    expect(HighlightHelper.getHighlight(undefined, undefined)).toEqual(
-      undefined
-    );
+    expect(getHighlight(null, null)).toEqual(undefined);
+    expect(getHighlight(undefined, undefined)).toEqual(undefined);
   });
 
   it('should return a style object when both parentNode and rangeToHighlight are valid', () => {
@@ -141,7 +111,7 @@ describe('#setNodeStyles', () => {
       y: 202,
     };
     const coords = { top: 65, left: 0 };
-    const highlight = HighlightHelper.getHighlight(rect, coords);
+    const highlight = getHighlight(rect, coords);
     expect(highlight).toEqual({
       style: {
         top: '62px',

diff --git a/src/callbacks/ContentEditableDiv.js b/src/callbacks/ContentEditableDiv.js
@@ -1,10 +1,18 @@
-const isEmailMessageBody = (mutation) =>
-  mutation.type === 'childList' &&
-  mutation.target.hasAttribute('contentEditable');
-
-export const ifEmailModified = (action) => (mutations) => {
-  const index = mutations.findIndex(isEmailMessageBody);
-  if (index > -1) {
-    action(mutations[index]);
-  }
+function isEmailMessageBody(mutation) {
+  return (
+    mutation.type === 'childList' &&
+    mutation.target.hasAttribute('contentEditable')
+  );
+}
+export const forEachUniqueContentEditable = (action) => {
+  const uniqueIds = new Set();
+  return (mutations) => {
+    mutations.filter(isEmailMessageBody).forEach((mutation) => {
+      if (!uniqueIds.has(mutation.target.id)) {
+        const id = mutation.target.id;
+        uniqueIds.add(id);
+        action(mutation);
+      }
+    });
+  };
 };