From bca3ac924bc893a6e5764aeb28cd8e034aadc00e Mon Sep 17 00:00:00 2001 From: James Duffy Date: Wed, 17 Feb 2021 15:54:25 -0500 Subject: [PATCH] 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. --- spec/ContentEditableDivSpec.test.js | 32 ++++--- spec/JustNotSorrySpec.test.js | 130 +++------------------------- spec/RangeFinderSpec.test.js | 27 ++++++ spec/UtilSpec.test.js | 2 +- spec/WarningSpec.test.js | 64 ++++---------- src/callbacks/ContentEditableDiv.js | 26 ++++-- src/components/JustNotSorry.js | 65 +++++++------- src/components/Warning.js | 90 +++++++++---------- src/helpers/RangeFinder.js | 17 ++++ src/{components => helpers}/util.js | 0 10 files changed, 181 insertions(+), 272 deletions(-) create mode 100644 spec/RangeFinderSpec.test.js create mode 100644 src/helpers/RangeFinder.js rename src/{components => helpers}/util.js (100%) diff --git a/spec/ContentEditableDivSpec.test.js b/spec/ContentEditableDivSpec.test.js index d37697d..fba5ae1 100644 --- 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 index 564232e..0d9bc65 100644 --- 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 new file mode 100644 index 0000000..dff3b9c --- /dev/null +++ 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 index e72b681..5164fea 100644 --- 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 index 28114e4..4f32202 100644 --- 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('', () => { }; beforeEach(() => { - wrapper = shallow(); + wrapper = shallow( + + ); }); it('should return a warning div', () => { @@ -66,10 +75,8 @@ describe('', () => { 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,10 +85,7 @@ 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, @@ -89,44 +93,10 @@ describe('#calculateCoords', () => { }); }); -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 index 01e50bd..af221c6 100644 --- 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); + } + }); + }; }; diff --git a/src/components/JustNotSorry.js b/src/components/JustNotSorry.js index 4eb32a9..368a4f1 100644 --- a/src/components/JustNotSorry.js +++ b/src/components/JustNotSorry.js @@ -1,9 +1,10 @@ import { h, Component } from 'preact'; import ReactDOM from 'react-dom'; import Warning from './Warning.js'; -import * as Util from './util.js'; +import * as Util from '../helpers/util.js'; import PHRASES from '../warnings/phrases.json'; -import { ifEmailModified } from '../callbacks/ContentEditableDiv.js'; +import { forEachUniqueContentEditable } from '../callbacks/ContentEditableDiv'; +import { findRanges } from '../helpers/RangeFinder'; const WAIT_TIME_BEFORE_RECALC_WARNINGS = 500; @@ -21,59 +22,55 @@ class JustNotSorry extends Component { constructor(props) { super(props); this.state = { + parentNode: {}, warnings: [], }; - this.applyEventListeners = this.applyEventListeners.bind(this); - this.resetState = this.resetState.bind(this); - this.searchEmail = this.searchEmail.bind(this); - this.search = this.search.bind(this); - this.handleSearch = (email) => - Util.debounce(this.searchEmail(email), WAIT_TIME_BEFORE_RECALC_WARNINGS); + this.resetState = this.resetState.bind(this); + this.handleSearch = this.handleSearch.bind(this); + this.updateWarnings = this.updateWarnings.bind(this); + this.applyEventListeners = this.applyEventListeners.bind(this); this.documentObserver = new MutationObserver( - ifEmailModified(this.applyEventListeners) + forEachUniqueContentEditable(this.applyEventListeners) ); this.documentObserver.observe(document.body, WATCH_FOR_NEW_NODES); } resetState() { - this.setState({ warnings: [] }); + this.setState({ parentNode: {}, warnings: [] }); } - search(email, phrase) { - return Util.match(email, phrase.regex).map((range) => ({ - message: phrase.message, - parentNode: email.parentNode, - rangeToHighlight: range, - })); + updateWarnings(elem, patterns) { + const newWarnings = findRanges(elem, patterns); + this.setState({ parentNode: elem.parentNode, warnings: newWarnings }); } - searchEmail(email) { - const newWarnings = []; - for (let i = 0; i < MESSAGE_PATTERNS.length; i++) { - const warnings = this.search(email, MESSAGE_PATTERNS[i]); - if (warnings.length > 0) { - newWarnings.push(...warnings); - } - } - this.setState({ warnings: newWarnings }); + handleSearch(elem, patterns) { + return Util.debounce( + () => this.updateWarnings(elem, patterns), + WAIT_TIME_BEFORE_RECALC_WARNINGS + ); } applyEventListeners(mutation) { const email = mutation.target; - email.addEventListener('input', () => this.handleSearch(email)); - email.addEventListener('focus', () => this.handleSearch(email)); - email.addEventListener('blur', () => this.resetState()); + const searchHandler = this.handleSearch(email, MESSAGE_PATTERNS); + email.addEventListener('input', searchHandler); + email.addEventListener('focus', searchHandler); + email.addEventListener('cut', searchHandler); + email.addEventListener('blur', this.resetState); } render() { - return this.state.warnings.map((warning, index) => - ReactDOM.createPortal( - , - warning.parentNode - ) - ); + if (this.state.warnings.length > 0) { + const parentNode = this.state.parentNode; + const parentRect = parentNode.getBoundingClientRect(); + const warnings = this.state.warnings.map((warning, index) => ( + + )); + return ReactDOM.createPortal(warnings, parentNode); + } } } diff --git a/src/components/Warning.js b/src/components/Warning.js index fee97fb..d10237b 100644 --- a/src/components/Warning.js +++ b/src/components/Warning.js @@ -1,63 +1,51 @@ import { h } from 'preact'; import WarningHighlight from './WarningHighlight.js'; -export class HighlightHelper { - static YPOS_ADJUSTMENT = 3; - static calculateCoords(parentRect, rect) { - return parentRect && rect - ? { - top: rect.top - parentRect.top + rect.height, - left: rect.left - parentRect.left, - } - : undefined; - } - - static getHighlight(rect, coord) { - return rect && coord - ? { - style: { - top: `${coord.top - this.YPOS_ADJUSTMENT}px`, - left: `${coord.left}px`, - width: `${rect.width}px`, - height: `${rect.height * 0.2}px`, - zIndex: 10, - position: 'absolute', - padding: '0px', - }, - position: coord.top <= 200 ? 'bottom' : 'top', - } - : undefined; - } +const YPOS_ADJUSTMENT = 3; +export function calculateCoords(parentRect, rect) { + return parentRect && rect + ? { + top: rect.top - parentRect.top + rect.height, + left: rect.left - parentRect.left, + } + : undefined; +} - static getHighlights(parentNode, rangeToHighlight) { - if (parentNode && rangeToHighlight) { - const parentRect = parentNode.getBoundingClientRect(); - return Array.from(rangeToHighlight.getClientRects(), (rect) => - this.getHighlight(rect, this.calculateCoords(parentRect, rect)) - ); - } - return undefined; - } +export function getHighlight(rect, coord) { + return rect && coord + ? { + style: { + top: `${coord.top - YPOS_ADJUSTMENT}px`, + left: `${coord.left}px`, + width: `${rect.width}px`, + height: `${rect.height * 0.2}px`, + zIndex: 10, + position: 'absolute', + padding: '0px', + }, + position: coord.top <= 200 ? 'bottom' : 'top', + } + : undefined; } export default function Warning(props) { - const { parentNode, rangeToHighlight } = props.value; - - const highlights = HighlightHelper.getHighlights( - parentNode, - rangeToHighlight - ); - + const rects = props.value.rangeToHighlight.getClientRects(); return (
- {highlights.map((highlight, index) => ( - - ))} + {Array.from(rects, (rect, index) => { + const highlight = getHighlight( + rect, + calculateCoords(props.parentRect, rect) + ); + return ( + + ); + })}
); } diff --git a/src/helpers/RangeFinder.js b/src/helpers/RangeFinder.js new file mode 100644 index 0000000..86199ba --- /dev/null +++ b/src/helpers/RangeFinder.js @@ -0,0 +1,17 @@ +import * as Util from './util'; + +function search(email, phrase) { + return Util.match(email, phrase.regex).map((range) => ({ + message: phrase.message, + rangeToHighlight: range, + })); +} + +export function findRanges(element, patternsToFind) { + const newWarnings = []; + for (let i = 0; i < patternsToFind.length; i++) { + const warnings = search(element, patternsToFind[i]); + newWarnings.push(...warnings); + } + return newWarnings; +} diff --git a/src/components/util.js b/src/helpers/util.js similarity index 100% rename from src/components/util.js rename to src/helpers/util.js