From c08407eec6cc10a58b55cc0ac081314ea768b9b3 Mon Sep 17 00:00:00 2001 From: Gregory Schier Date: Thu, 10 Nov 2016 13:03:12 -0800 Subject: [PATCH] Some refactoring of debouncing --- .../{localstorage.js => LocalStorage.js} | 0 ...alstorage.test.js => LocalStorage.test.js} | 2 +- app/common/__tests__/misc.test.js | 122 ++++++++++++++---- app/common/database.js | 3 +- app/common/misc.js | 24 +++- app/main.development.js | 2 +- app/sync/index.js | 80 ++++-------- app/ui/components/base/DebouncedInput.js | 26 ++++ app/ui/components/base/Editable.js | 15 ++- app/ui/components/base/Editor.js | 17 +-- app/ui/components/modals/CookiesModal.js | 9 +- 11 files changed, 188 insertions(+), 112 deletions(-) rename app/common/{localstorage.js => LocalStorage.js} (100%) rename app/common/__tests__/{localstorage.test.js => LocalStorage.test.js} (98%) create mode 100644 app/ui/components/base/DebouncedInput.js diff --git a/app/common/localstorage.js b/app/common/LocalStorage.js similarity index 100% rename from app/common/localstorage.js rename to app/common/LocalStorage.js diff --git a/app/common/__tests__/localstorage.test.js b/app/common/__tests__/LocalStorage.test.js similarity index 98% rename from app/common/__tests__/localstorage.test.js rename to app/common/__tests__/LocalStorage.test.js index 1a0755876..44348798e 100644 --- a/app/common/__tests__/localstorage.test.js +++ b/app/common/__tests__/LocalStorage.test.js @@ -1,6 +1,6 @@ import path from 'path'; import fs from 'fs'; -import LocalStorage from '../localstorage'; +import LocalStorage from '../LocalStorage'; describe('LocalStorage()', () => { beforeEach(() => { diff --git a/app/common/__tests__/misc.test.js b/app/common/__tests__/misc.test.js index da6379876..2e0f830d3 100644 --- a/app/common/__tests__/misc.test.js +++ b/app/common/__tests__/misc.test.js @@ -1,8 +1,8 @@ -import * as util from '../misc'; +import * as misc from '../misc'; describe('getBasicAuthHeader()', () => { it('succeed with username and password', () => { - const header = util.getBasicAuthHeader('user', 'password'); + const header = misc.getBasicAuthHeader('user', 'password'); expect(header).toEqual({ name: 'Authorization', value: 'Basic dXNlcjpwYXNzd29yZA==' @@ -10,7 +10,7 @@ describe('getBasicAuthHeader()', () => { }); it('succeed with no username', () => { - const header = util.getBasicAuthHeader(null, 'password'); + const header = misc.getBasicAuthHeader(null, 'password'); expect(header).toEqual({ name: 'Authorization', value: 'Basic OnBhc3N3b3Jk' @@ -18,7 +18,7 @@ describe('getBasicAuthHeader()', () => { }); it('succeed with username and empty password', () => { - const header = util.getBasicAuthHeader('user', ''); + const header = misc.getBasicAuthHeader('user', ''); expect(header).toEqual({ name: 'Authorization', value: 'Basic dXNlcjo=' @@ -26,7 +26,7 @@ describe('getBasicAuthHeader()', () => { }); it('succeed with username and null password', () => { - const header = util.getBasicAuthHeader('user', null); + const header = misc.getBasicAuthHeader('user', null); expect(header).toEqual({ name: 'Authorization', value: 'Basic dXNlcjo=' @@ -36,7 +36,7 @@ describe('getBasicAuthHeader()', () => { describe('hasAuthHeader()', () => { it('finds valid header', () => { - const yes = util.hasAuthHeader([ + const yes = misc.hasAuthHeader([ {name: 'foo', value: 'bar'}, {name: 'authorization', value: 'foo'} ]); @@ -45,7 +45,7 @@ describe('hasAuthHeader()', () => { }); it('finds valid header case insensitive', () => { - const yes = util.hasAuthHeader([ + const yes = misc.hasAuthHeader([ {name: 'foo', value: 'bar'}, {name: 'AuthOrizAtiOn', value: 'foo'} ]); @@ -56,89 +56,155 @@ describe('hasAuthHeader()', () => { describe('generateId()', () => { it('generates a valid ID', () => { - const id = util.generateId('foo'); + const id = misc.generateId('foo'); expect(id).toMatch(/^foo_[a-z0-9]{32}$/); }); it('generates without prefix', () => { - const id = util.generateId(); + const id = misc.generateId(); expect(id).toMatch(/^[a-z0-9]{32}$/); }); }); describe('setDefaultProtocol()', () => { it('correctly sets protocol for empty', () => { - const url = util.setDefaultProtocol('google.com'); + const url = misc.setDefaultProtocol('google.com'); expect(url).toBe('http://google.com'); }); it('does not set for valid url', () => { - const url = util.setDefaultProtocol('https://google.com'); + const url = misc.setDefaultProtocol('https://google.com'); expect(url).toBe('https://google.com'); }); it('does not set for valid url', () => { - const url = util.setDefaultProtocol('http://google.com'); + const url = misc.setDefaultProtocol('http://google.com'); expect(url).toBe('http://google.com'); }); it('does not set for invalid url', () => { - const url = util.setDefaultProtocol('httbad://google.com'); + const url = misc.setDefaultProtocol('httbad://google.com'); expect(url).toBe('httbad://google.com'); }); }); describe('prepareUrlForSending()', () => { it('does not touch normal url', () => { - const url = util.prepareUrlForSending('http://google.com'); + const url = misc.prepareUrlForSending('http://google.com'); expect(url).toBe('http://google.com/'); }); it('works with no protocol', () => { - const url = util.prepareUrlForSending('google.com'); + const url = misc.prepareUrlForSending('google.com'); expect(url).toBe('http://google.com/'); }); it('encodes pathname', () => { - const url = util.prepareUrlForSending('https://google.com/foo bar/100%/foo'); + const url = misc.prepareUrlForSending('https://google.com/foo bar/100%/foo'); expect(url).toBe('https://google.com/foo%20bar/100%25/foo'); }); it('encodes pathname mixed encoding', () => { - const url = util.prepareUrlForSending('https://google.com/foo bar baz%20qux/100%/foo%25'); + const url = misc.prepareUrlForSending('https://google.com/foo bar baz%20qux/100%/foo%25'); expect(url).toBe('https://google.com/foo%20bar%20baz%20qux/100%25/foo%25'); }); it('leaves already encoded pathname', () => { - const url = util.prepareUrlForSending('https://google.com/foo%20bar%20baz/100%25/foo'); + const url = misc.prepareUrlForSending('https://google.com/foo%20bar%20baz/100%25/foo'); expect(url).toBe('https://google.com/foo%20bar%20baz/100%25/foo'); }); it('encodes querystring', () => { - const url = util.prepareUrlForSending('https://google.com?s=foo bar 100%&hi'); + const url = misc.prepareUrlForSending('https://google.com?s=foo bar 100%&hi'); expect(url).toBe('https://google.com/?s=foo%20bar%20100%25&hi='); }); it('encodes querystring with mixed spaces', () => { - const url = util.prepareUrlForSending('https://google.com?s=foo %20100%'); + const url = misc.prepareUrlForSending('https://google.com?s=foo %20100%'); expect(url).toBe('https://google.com/?s=foo%20%20100%25'); }); it('encodes querystring with repeated keys', () => { - const url = util.prepareUrlForSending('https://google.com?s=foo&s=foo %20100%'); + const url = misc.prepareUrlForSending('https://google.com?s=foo&s=foo %20100%'); expect(url).toBe('https://google.com/?s=foo&s=foo%20%20100%25'); }); }); describe('filterHeaders()', () => { it('handles bad headers', () => { - expect(util.filterHeaders(null, null)).toEqual([]); - expect(util.filterHeaders([], null)).toEqual([]); - expect(util.filterHeaders(['bad'], null)).toEqual([]); - expect(util.filterHeaders(['bad'], 'good')).toEqual([]); - expect(util.filterHeaders(null, 'good')).toEqual([]); - expect(util.filterHeaders([{name: 'good', value: 'valid'}], null)).toEqual([]); - expect(util.filterHeaders([{name: 'good', value: 'valid'}], 'good')) + expect(misc.filterHeaders(null, null)).toEqual([]); + expect(misc.filterHeaders([], null)).toEqual([]); + expect(misc.filterHeaders(['bad'], null)).toEqual([]); + expect(misc.filterHeaders(['bad'], 'good')).toEqual([]); + expect(misc.filterHeaders(null, 'good')).toEqual([]); + expect(misc.filterHeaders([{name: 'good', value: 'valid'}], null)).toEqual([]); + expect(misc.filterHeaders([{name: 'good', value: 'valid'}], 'good')) .toEqual([{name: 'good', value: 'valid'}]); }) }); + +describe('keyedDebounce()', () => { + beforeEach(() => { + jest.useFakeTimers(); + + // There has to be a better way to reset this... + setTimeout.mock.calls = []; + }); + + afterEach(() => jest.clearAllTimers()); + + it('debounces correctly', () => { + const resultsList = []; + const fn = misc.keyedDebounce(results => { + resultsList.push(results); + }, 100); + + fn('foo', 'bar'); + fn('baz', 'bar'); + fn('foo', 'bar2'); + fn('foo', 'bar3'); + fn('multi', 'foo', 'bar', 'baz'); + + expect(setTimeout.mock.calls.length).toBe(5); + expect(resultsList).toEqual([]); + + jest.runAllTimers(); + + expect(resultsList).toEqual([{ + foo: ['bar3'], + baz: ['bar'], + multi: ['foo', 'bar', 'baz'] + }]); + }) +}); + +describe('debounce()', () => { + beforeEach(() => { + jest.useFakeTimers(); + + // There has to be a better way to reset this... + setTimeout.mock.calls = []; + }); + + afterEach(() => jest.clearAllTimers()); + + it('debounces correctly', () => { + const resultList = []; + const fn = misc.debounce((...args) => { + resultList.push(args); + }, 100); + + fn('foo'); + fn('foo'); + fn('multi', 'foo', 'bar', 'baz'); + fn('baz', 'bar'); + fn('foo', 'bar3'); + + expect(setTimeout.mock.calls.length).toBe(5); + expect(resultList).toEqual([]); + + jest.runAllTimers(); + + expect(resultList).toEqual([['foo', 'bar3']]); + }) +}); diff --git a/app/common/database.js b/app/common/database.js index accf85489..48ab88946 100644 --- a/app/common/database.js +++ b/app/common/database.js @@ -55,8 +55,9 @@ export async function initDB (types, config = {}, forceReset = false) { db[modelType].persistence.setAutocompactionInterval(DB_PERSIST_INTERVAL); - console.log(`-- Initialized DB at ${filePath} --`); } + + console.log(`-- Initialized DB at ${getDBFilePath('$TYPE')} --`); } diff --git a/app/common/misc.js b/app/common/misc.js index bb9cb1b46..bf2bf63b3 100644 --- a/app/common/misc.js +++ b/app/common/misc.js @@ -101,7 +101,7 @@ export function prepareUrlForSending (url) { return urlFormat(parsedUrl); } -export function delay (milliseconds) { +export function delay (milliseconds = DEBOUNCE_MILLIS) { return new Promise(resolve => setTimeout(resolve, milliseconds)) } @@ -109,12 +109,28 @@ export function removeVowels (str) { return str.replace(/[aeiouyAEIOUY]/g, ''); } -export function debounce (callback, millis = DEBOUNCE_MILLIS) { +export function keyedDebounce (callback, millis = DEBOUNCE_MILLIS) { let timeout = null; - return function () { + let results = {}; + + return function (key, ...args) { + results[key] = args; + clearTimeout(timeout); timeout = setTimeout(() => { - callback.apply(null, arguments) + if (!Object.keys(results).length) { + return; + } + + callback(results); + results = {}; }, millis); } } + +export function debounce (callback, millis = DEBOUNCE_MILLIS) { + // For regular debounce, just use a keyed debounce with a fixed key + return keyedDebounce(results => { + callback.apply(null, results['__key__']) + }, millis).bind(null, '__key__'); +} diff --git a/app/main.development.js b/app/main.development.js index 018dd67c9..82fa40f6e 100644 --- a/app/main.development.js +++ b/app/main.development.js @@ -7,7 +7,7 @@ import request from 'request'; import path from 'path'; import electron from 'electron'; import * as packageJSON from './package.json'; -import LocalStorage from './common/localstorage'; +import LocalStorage from './common/LocalStorage'; // Some useful helpers const IS_DEV = process.env.INSOMNIA_ENV === 'development'; diff --git a/app/sync/index.js b/app/sync/index.js index aa602facf..6a52530d6 100644 --- a/app/sync/index.js +++ b/app/sync/index.js @@ -5,6 +5,7 @@ import * as crypt from './crypt'; import * as session from './session'; import * as store from './storage'; import Logger from './logger'; +import * as misc from '../common/misc'; export const FULL_SYNC_INTERVAL = 60E3; export const QUEUE_DEBOUNCE_TIME = 1E3; @@ -29,17 +30,17 @@ const resourceGroupSymmetricKeysCache = {}; let isInitialized = false; export async function initSync () { - const settings = await models.settings.getOrCreate(); - if (!settings.optSyncBeta) { - logger.debug('Not enabled'); - return; - } - if (isInitialized) { logger.debug('Already enabled'); return; } + const _queueChange = misc.keyedDebounce(results => { + for (const k of Object.keys(results)) { + _pushChange(...results[k]); + } + }, QUEUE_DEBOUNCE_TIME); + db.onChange(changes => { for (const [event, doc, fromSync] of changes) { if (!WHITE_LIST[doc.type]) { @@ -52,7 +53,8 @@ export async function initSync () { } // Make sure it happens async - process.nextTick(() => _queueChange(event, doc, fromSync)); + const key = `${event}:${doc._id}`; + _queueChange(key, event, doc, Date.now()); } }); @@ -385,54 +387,28 @@ export async function resetRemoteData () { // HELPERS // // ~~~~~~~ // -let _queuedChanges = {}; -let _queuedChangesTimeout = null; let _pushChangesTimeout = null; +async function _pushChange (event, doc, timestamp) { + // Update the resource content and set dirty + // TODO: Remove one of these steps since it does encryption twice + // in the case where the resource does not exist yet + const resource = await getOrCreateResourceForDoc(doc); -async function _queueChange (event, doc) { - if (!session.isLoggedIn()) { - logger.warn('Not logged in'); - return; - } + const updatedResource = await store.updateResource(resource, { + name: doc.name || 'n/a', + lastEdited: timestamp, + lastEditedBy: session.getAccountId(), + encContent: await _encryptDoc(resource.resourceGroupId, doc), + removed: event === db.CHANGE_REMOVE, + dirty: true + }); - // How this works? - // First, debounce updates to Resources because they are heavy (encryption) - // Second, debounce pushes to the server, because they are slow (network) - // ... Using _queuedChanges as a map so that future changes to the same doc - // don't trigger more than 1 update. - - // NOTE: Don't use doc.modified because that doesn't work for removal - _queuedChanges[doc._id + event] = [event, doc, Date.now()]; - - clearTimeout(_queuedChangesTimeout); - _queuedChangesTimeout = setTimeout(async () => { - - const queuedChangesCopy = Object.assign({}, _queuedChanges); - _queuedChanges = {}; - - for (const k of Object.keys(queuedChangesCopy)) { - const [event, doc, timestamp] = queuedChangesCopy[k]; - - // Update the resource content and set dirty - // TODO: Remove one of these steps since it does encryption twice - // in the case where the resource does not exist yet - const resource = await getOrCreateResourceForDoc(doc); - - const updatedResource = await store.updateResource(resource, { - name: doc.name || 'n/a', - lastEdited: timestamp, - lastEditedBy: session.getAccountId(), - encContent: await _encryptDoc(resource.resourceGroupId, doc), - removed: event === db.CHANGE_REMOVE, - dirty: true - }); - - // Debounce pushing of dirty resources - logger.debug(`Queue ${event} ${updatedResource.id}`); - clearTimeout(_pushChangesTimeout); - _pushChangesTimeout = setTimeout(() => pushActiveDirtyResources(), PUSH_DEBOUNCE_TIME); - } - }, QUEUE_DEBOUNCE_TIME); + // Debounce pushing of dirty resources + logger.debug(`Queue ${event} ${updatedResource.id}`); + clearTimeout(_pushChangesTimeout); + _pushChangesTimeout = setTimeout(() => { + pushActiveDirtyResources() + }, PUSH_DEBOUNCE_TIME); } /** diff --git a/app/ui/components/base/DebouncedInput.js b/app/ui/components/base/DebouncedInput.js new file mode 100644 index 000000000..e8bd95b81 --- /dev/null +++ b/app/ui/components/base/DebouncedInput.js @@ -0,0 +1,26 @@ +import React, {Component, PropTypes} from 'react'; +import * as misc from '../../../common/misc'; + +class DebouncedInput extends Component { + constructor (props) { + super(props); + + // NOTE: Cannot debounce props.onChange directly because react events aren't + // allowed to be used after a timeout :( + const debouncedOnChange = misc.debounce(value => this.props.onChange(value)); + const onChangeFn = e => debouncedOnChange(e.target.value); + this._onChange = onChangeFn.bind(this); + } + + render () { + // NOTE: Strip out onChange because we're overriding it + const {onChange, ...props} = this.props; + return + } +} + +DebouncedInput.propTypes = { + onChange: PropTypes.func.isRequired, +}; + +export default DebouncedInput; diff --git a/app/ui/components/base/Editable.js b/app/ui/components/base/Editable.js index fa5684444..ba86dd03a 100644 --- a/app/ui/components/base/Editable.js +++ b/app/ui/components/base/Editable.js @@ -1,5 +1,5 @@ import React, {Component, PropTypes} from 'react'; -import {DEBOUNCE_MILLIS} from '../../../common/constants'; +import * as misc from '../../../common/misc'; class Editable extends Component { constructor (props) { @@ -18,7 +18,7 @@ class Editable extends Component { }); } - _handleEditEnd () { + async _handleEditEnd () { const value = this._input.value.trim(); if (!value) { @@ -26,13 +26,12 @@ class Editable extends Component { return; } + this.props.onSubmit(value); + // This timeout prevents the UI from showing the old value after submit. // It should give the UI enough time to redraw the new value. - setTimeout(() => { - this.setState({editing: false}); - }, DEBOUNCE_MILLIS); - - this.props.onSubmit(value); + await misc.delay(100); + this.setState({editing: false}); } _handleEditKeyDown (e) { @@ -41,6 +40,8 @@ class Editable extends Component { this._handleEditEnd(); } else if (e.keyCode === 27) { // Pressed Escape + // NOTE: This blur causes a save because we save on blur + // TODO: Make escape blur without saving this._input && this._input.blur(); } } diff --git a/app/ui/components/base/Editor.js b/app/ui/components/base/Editor.js index 8b2e2b025..5704cb719 100644 --- a/app/ui/components/base/Editor.js +++ b/app/ui/components/base/Editor.js @@ -42,7 +42,7 @@ import '../../css/components/editor.less'; import {showModal} from '../modals/index'; import AlertModal from '../modals/AlertModal'; import Link from '../base/Link'; -import {DEBOUNCE_MILLIS} from '../../../common/constants'; +import * as misc from '../../../common/misc'; const BASE_CODEMIRROR_OPTIONS = { @@ -122,8 +122,8 @@ class Editor extends Component { const {value} = this.props; this.codeMirror = CodeMirror.fromTextArea(textarea, BASE_CODEMIRROR_OPTIONS); - this.codeMirror.on('change', this._codemirrorValueChanged.bind(this)); - this.codeMirror.on('paste', this._codemirrorValueChanged.bind(this)); + this.codeMirror.on('change', misc.debounce(this._codemirrorValueChanged.bind(this))); + this.codeMirror.on('paste', misc.debounce(this._codemirrorValueChanged.bind(this))); if (!this.codeMirror.getOption('indentWithTabs')) { this.codeMirror.setOption('extraKeys', { Tab: cm => { @@ -248,13 +248,8 @@ class Editor extends Component { return; } - // Debounce URL changes so we don't update the app so much - clearTimeout(this._askTimeout); - this._askTimeout = setTimeout(() => { - // Update our cached value - var newValue = doc.getValue(); - this.props.onChange(newValue); - }, DEBOUNCE_MILLIS) + var newValue = doc.getValue(); + this.props.onChange(newValue); } /** @@ -280,7 +275,7 @@ class Editor extends Component { if (this.props.updateFilter) { this.props.updateFilter(filter); } - }, DEBOUNCE_MILLIS * 3); + }, 400); } _canPrettify () { diff --git a/app/ui/components/modals/CookiesModal.js b/app/ui/components/modals/CookiesModal.js index 17f628d9c..3ae4a6278 100644 --- a/app/ui/components/modals/CookiesModal.js +++ b/app/ui/components/modals/CookiesModal.js @@ -1,12 +1,10 @@ import React, {PropTypes, Component} from 'react'; - import Modal from '../base/Modal'; import ModalBody from '../base/ModalBody'; import ModalHeader from '../base/ModalHeader'; import ModalFooter from '../base/ModalFooter'; import CookiesEditor from '../editors/CookiesEditor'; import * as models from '../../../models'; -import {DEBOUNCE_MILLIS} from '../../../common/constants'; class CookiesModal extends Component { constructor (props) { @@ -15,7 +13,7 @@ class CookiesModal extends Component { cookieJar: null, workspace: null, filter: '' - } + }; } async _saveChanges () { @@ -56,10 +54,7 @@ class CookiesModal extends Component { } _onFilterChange (filter) { - clearTimeout(this._askTimeout); - this._askTimeout = setTimeout(() => { - this.setState({filter}); - }, DEBOUNCE_MILLIS); + this.setState({filter}); } _getFilteredSortedCookies () {