From 753b37cde65c760947f067a0d02244ba4767f2b4 Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Tue, 7 Nov 2017 14:57:13 -0800 Subject: [PATCH 1/8] Throttle calls to "window.history.replaceState" to fix Mobile Safari error message fixes #5612 --- package.json | 1 + src/source/tile.js | 1 - src/ui/hash.js | 7 ++++- src/util/throttler.js | 59 ------------------------------------------- yarn.lock | 4 +++ 5 files changed, 11 insertions(+), 61 deletions(-) delete mode 100644 src/util/throttler.js diff --git a/package.json b/package.json index 4e06984bf9e..eb0b3ea77ee 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ "grid-index": "^1.0.0", "jsonlint-lines-primitives": "~1.6.0", "lodash.isequal": "^3.0.4", + "lodash.throttle": "^4.1.1", "mapbox-gl-supported": "^1.2.0", "minimist": "0.0.8", "package-json-versionify": "^1.0.2", diff --git a/src/source/tile.js b/src/source/tile.js index 2bb9ed0ec33..ee8032845fe 100644 --- a/src/source/tile.js +++ b/src/source/tile.js @@ -61,7 +61,6 @@ class Tile { expirationTime: any; expiredRequestCount: number; state: TileState; - placementThrottler: any; timeAdded: any; fadeEndTime: any; rawTileData: ArrayBuffer; diff --git a/src/ui/hash.js b/src/ui/hash.js index f79a0e4dad4..6635c8fc089 100644 --- a/src/ui/hash.js +++ b/src/ui/hash.js @@ -2,6 +2,7 @@ const util = require('../util/util'); const window = require('../util/window'); +const throttle = require('lodash.throttle'); import type Map from './map'; @@ -13,12 +14,16 @@ import type Map from './map'; */ class Hash { _map: Map; + _updateHash: () => void; constructor() { util.bindAll([ '_onHashChange', '_updateHash' ], this); + + // Mobile Safari doesn't allow updating the hash more than 100 times per 30 seconds. + this._updateHash = throttle(this._updateHashUnthrottled.bind(this), 30 * 1000 / 100); } /* @@ -82,7 +87,7 @@ class Hash { return false; } - _updateHash() { + _updateHashUnthrottled() { const hash = this.getHashString(); window.history.replaceState('', '', hash); } diff --git a/src/util/throttler.js b/src/util/throttler.js deleted file mode 100644 index f25638a6723..00000000000 --- a/src/util/throttler.js +++ /dev/null @@ -1,59 +0,0 @@ -// @flow - -const browser = require('./browser'); - -/** - * Throttles the provided function to run at most every - * 'frequency' milliseconds - * - * @interface Throttler - * @private - */ -class Throttler { - frequency: number; - throttledFunction: () => void; - lastInvocation: number; - pendingInvocation: ?number; - - constructor(frequency: number, throttledFunction: () => void) { - this.frequency = frequency; - this.throttledFunction = throttledFunction; - this.lastInvocation = 0; - } - - /** - * Request an invocation of the throttled function. - * - * @memberof Throttler - * @instance - */ - invoke() { - if (this.pendingInvocation) { - return; - } - - const timeToNextInvocation = this.lastInvocation === 0 ? - 0 : - (this.lastInvocation + this.frequency) - browser.now(); - - if (timeToNextInvocation <= 0) { - this.lastInvocation = browser.now(); - this.throttledFunction(); - } else { - this.pendingInvocation = setTimeout(() => { - this.pendingInvocation = undefined; - this.lastInvocation = browser.now(); - this.throttledFunction(); - }, timeToNextInvocation); - } - } - - stop() { - if (this.pendingInvocation) { - clearTimeout(this.pendingInvocation); - this.pendingInvocation = undefined; - } - } -} - -module.exports = Throttler; diff --git a/yarn.lock b/yarn.lock index 30603286fb2..86af401a15b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6189,6 +6189,10 @@ lodash.templatesettings@^3.0.0: lodash._reinterpolate "^3.0.0" lodash.escape "^3.0.0" +lodash.throttle@^4.1.1: + version "4.1.1" + resolved "https://registry.yarnpkg.com/lodash.throttle/-/lodash.throttle-4.1.1.tgz#c23e91b710242ac70c37f1e1cda9274cc39bf2f4" + lodash.toplainobject@^3.0.0: version "3.0.0" resolved "https://registry.yarnpkg.com/lodash.toplainobject/-/lodash.toplainobject-3.0.0.tgz#28790ad942d293d78aa663a07ecf7f52ca04198d" From 45cf1586c870afd40d158d038ce2ad89cd57cc76 Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Wed, 8 Nov 2017 14:27:48 -0800 Subject: [PATCH 2/8] Fix unit tests --- src/ui/hash.js | 5 +++-- test/unit/ui/hash.test.js | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ui/hash.js b/src/ui/hash.js index 6635c8fc089..3e521d6f28b 100644 --- a/src/ui/hash.js +++ b/src/ui/hash.js @@ -16,14 +16,15 @@ class Hash { _map: Map; _updateHash: () => void; - constructor() { + constructor(options: ?{throttle?: Boolean;}) { util.bindAll([ '_onHashChange', '_updateHash' ], this); // Mobile Safari doesn't allow updating the hash more than 100 times per 30 seconds. - this._updateHash = throttle(this._updateHashUnthrottled.bind(this), 30 * 1000 / 100); + const throttleTime = (options && options.throttle === false) ? 0 : 30 * 1000 / 100; + this._updateHash = throttle(this._updateHashUnthrottled.bind(this), throttleTime); } /* diff --git a/test/unit/ui/hash.test.js b/test/unit/ui/hash.test.js index b4a521c271f..b7805added4 100644 --- a/test/unit/ui/hash.test.js +++ b/test/unit/ui/hash.test.js @@ -7,7 +7,7 @@ const Map = require('../../../src/ui/map'); test('hash', (t) => { function createHash() { - return new Hash(); + return new Hash({throttle: false}); } function createMap() { From 3ac5097e8579183d35f5ac407061dd9d40620ecc Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Wed, 8 Nov 2017 15:59:56 -0800 Subject: [PATCH 3/8] Use custom throttle function --- debug/index.html | 6 ++++ package.json | 1 - src/ui/hash.js | 2 +- src/util/throttle.js | 35 +++++++++++++++++++++++ src/util/util.js | 26 +++++++++++++++++ test/unit/util/throttle.test.js | 49 +++++++++++++++++++++++++++++++++ yarn.lock | 4 --- 7 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 src/util/throttle.js create mode 100644 test/unit/util/throttle.test.js diff --git a/debug/index.html b/debug/index.html index 9c7f3bb641c..35301f03185 100644 --- a/debug/index.html +++ b/debug/index.html @@ -26,6 +26,12 @@ hash: true }); +setInterval(() => { + map.flyTo({ + bearing: map.getBearing() + 1 + }); +}, 100); + diff --git a/package.json b/package.json index eb0b3ea77ee..4e06984bf9e 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,6 @@ "grid-index": "^1.0.0", "jsonlint-lines-primitives": "~1.6.0", "lodash.isequal": "^3.0.4", - "lodash.throttle": "^4.1.1", "mapbox-gl-supported": "^1.2.0", "minimist": "0.0.8", "package-json-versionify": "^1.0.2", diff --git a/src/ui/hash.js b/src/ui/hash.js index 3e521d6f28b..82f026de2d3 100644 --- a/src/ui/hash.js +++ b/src/ui/hash.js @@ -2,7 +2,7 @@ const util = require('../util/util'); const window = require('../util/window'); -const throttle = require('lodash.throttle'); +const throttle = require('../util/throttle'); import type Map from './map'; diff --git a/src/util/throttle.js b/src/util/throttle.js new file mode 100644 index 00000000000..e54ce241f54 --- /dev/null +++ b/src/util/throttle.js @@ -0,0 +1,35 @@ +// @flow + +/** + * Throttle the given function to run at most every `period` milliseconds. + */ +module.exports = function(unthrottledFunction: () => void, period: number): () => void { + + // The next time (unix epoch) that the function is allowed to execute + let nextTime = 0; + + // `true` if there is a pending "setTimeout" operation that'll invoke the + // function at a later time. `false` if there is not. + let pending = false; + + let throttledFunction = () => { + const time = Date.now(); + + if (nextTime <= time && !pending) { + nextTime = time + period; + unthrottledFunction(); + } else if (!pending) { + pending = true; + setTimeout(_throttledFunction, nextTime - time); + } + }; + + // This callback to `setTimeout` is written outside `throttledFunction` to + // reduce the number of closures created. + let _throttledFunction = () => { + pending = false; + throttledFunction(); + }; + + return throttledFunction; +}; diff --git a/src/util/util.js b/src/util/util.js index 540e65f43d2..0c73f5df541 100644 --- a/src/util/util.js +++ b/src/util/util.js @@ -457,3 +457,29 @@ exports.parseCacheControl = function(cacheControl: string): Object { return header; }; + +/** + * Throttle the given function to run at most every `time` milliseconds. + */ +exports.throttle = function(fn: () => void, time: number): () => number { + let pending = false; + let timerId = 0; + + const later = () => { + timerId = 0; + if (pending) { + fn(); + pending = false; + } + }; + + return () => { + if (timerId) { + pending = true; + } else { + fn(); + timerId = setTimeout(later, time); + } + return timerId; + }; +}; diff --git a/test/unit/util/throttle.test.js b/test/unit/util/throttle.test.js new file mode 100644 index 00000000000..74e86c39140 --- /dev/null +++ b/test/unit/util/throttle.test.js @@ -0,0 +1,49 @@ +'use strict'; +// @flow + +const test = require('mapbox-gl-js-test').test; +const throttle = require('../../../src/util/throttle'); + +test('throttle', (t) => { + + t.test('does not execute unthrottled function unless throttled function is invoked', (t) => { + let executionCount = 0; + let throttledFunction = throttle(() => executionCount++, 0); + t.equal(executionCount, 0); + t.end(); + }); + + t.test('executes unthrottled function immediately when period is 0', (t) => { + let executionCount = 0; + let throttledFunction = throttle(() => executionCount++, 0); + throttledFunction(); + throttledFunction(); + throttledFunction(); + t.equal(executionCount, 3); + t.end(); + }); + + t.test('executes unthrottled function immediately once when period is > 0', (t) => { + let executionCount = 0; + let throttledFunction = throttle(() => executionCount++, 5); + throttledFunction(); + throttledFunction(); + throttledFunction(); + t.equal(executionCount, 1); + t.end(); + }); + + t.test('queues exactly one execution of unthrottled function when period is > 0', (t) => { + let executionCount = 0; + let throttledFunction = throttle(() => executionCount++, 5); + throttledFunction(); + throttledFunction(); + throttledFunction(); + setTimeout(() => { + t.equal(executionCount, 2); + t.end(); + }, 10); + }); + + t.end(); +}); diff --git a/yarn.lock b/yarn.lock index 86af401a15b..30603286fb2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6189,10 +6189,6 @@ lodash.templatesettings@^3.0.0: lodash._reinterpolate "^3.0.0" lodash.escape "^3.0.0" -lodash.throttle@^4.1.1: - version "4.1.1" - resolved "https://registry.yarnpkg.com/lodash.throttle/-/lodash.throttle-4.1.1.tgz#c23e91b710242ac70c37f1e1cda9274cc39bf2f4" - lodash.toplainobject@^3.0.0: version "3.0.0" resolved "https://registry.yarnpkg.com/lodash.toplainobject/-/lodash.toplainobject-3.0.0.tgz#28790ad942d293d78aa663a07ecf7f52ca04198d" From 7c0da67abd8052fecac47ff6a0c42c179e5fc766 Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Wed, 8 Nov 2017 16:36:44 -0800 Subject: [PATCH 4/8] Appease the linter gods --- src/util/throttle.js | 5 +++-- test/unit/util/throttle.test.js | 8 ++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/util/throttle.js b/src/util/throttle.js index e54ce241f54..15ffceee0eb 100644 --- a/src/util/throttle.js +++ b/src/util/throttle.js @@ -12,7 +12,7 @@ module.exports = function(unthrottledFunction: () => void, period: number): () = // function at a later time. `false` if there is not. let pending = false; - let throttledFunction = () => { + const throttledFunction = () => { const time = Date.now(); if (nextTime <= time && !pending) { @@ -20,13 +20,14 @@ module.exports = function(unthrottledFunction: () => void, period: number): () = unthrottledFunction(); } else if (!pending) { pending = true; + // eslint-disable-next-line no-use-before-define setTimeout(_throttledFunction, nextTime - time); } }; // This callback to `setTimeout` is written outside `throttledFunction` to // reduce the number of closures created. - let _throttledFunction = () => { + const _throttledFunction = () => { pending = false; throttledFunction(); }; diff --git a/test/unit/util/throttle.test.js b/test/unit/util/throttle.test.js index 74e86c39140..11a684c3f8c 100644 --- a/test/unit/util/throttle.test.js +++ b/test/unit/util/throttle.test.js @@ -8,14 +8,14 @@ test('throttle', (t) => { t.test('does not execute unthrottled function unless throttled function is invoked', (t) => { let executionCount = 0; - let throttledFunction = throttle(() => executionCount++, 0); + throttle(() => { executionCount++; }, 0); t.equal(executionCount, 0); t.end(); }); t.test('executes unthrottled function immediately when period is 0', (t) => { let executionCount = 0; - let throttledFunction = throttle(() => executionCount++, 0); + const throttledFunction = throttle(() => { executionCount++; }, 0); throttledFunction(); throttledFunction(); throttledFunction(); @@ -25,7 +25,7 @@ test('throttle', (t) => { t.test('executes unthrottled function immediately once when period is > 0', (t) => { let executionCount = 0; - let throttledFunction = throttle(() => executionCount++, 5); + const throttledFunction = throttle(() => { executionCount++; }, 5); throttledFunction(); throttledFunction(); throttledFunction(); @@ -35,7 +35,7 @@ test('throttle', (t) => { t.test('queues exactly one execution of unthrottled function when period is > 0', (t) => { let executionCount = 0; - let throttledFunction = throttle(() => executionCount++, 5); + const throttledFunction = throttle(() => { executionCount++; }, 5); throttledFunction(); throttledFunction(); throttledFunction(); From 3eb7a25435aebdb522910f96256661626f974dac Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Thu, 9 Nov 2017 14:24:56 -0800 Subject: [PATCH 5/8] Revert changes to debug/index.html --- debug/index.html | 6 ------ 1 file changed, 6 deletions(-) diff --git a/debug/index.html b/debug/index.html index 35301f03185..9c7f3bb641c 100644 --- a/debug/index.html +++ b/debug/index.html @@ -26,12 +26,6 @@ hash: true }); -setInterval(() => { - map.flyTo({ - bearing: map.getBearing() + 1 - }); -}, 100); - From ebc326ebcf45f7a8c622cba5a1017ee4a38c9d8f Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Thu, 9 Nov 2017 14:26:22 -0800 Subject: [PATCH 6/8] Revert changes to util.js --- src/util/util.js | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/src/util/util.js b/src/util/util.js index 0c73f5df541..540e65f43d2 100644 --- a/src/util/util.js +++ b/src/util/util.js @@ -457,29 +457,3 @@ exports.parseCacheControl = function(cacheControl: string): Object { return header; }; - -/** - * Throttle the given function to run at most every `time` milliseconds. - */ -exports.throttle = function(fn: () => void, time: number): () => number { - let pending = false; - let timerId = 0; - - const later = () => { - timerId = 0; - if (pending) { - fn(); - pending = false; - } - }; - - return () => { - if (timerId) { - pending = true; - } else { - fn(); - timerId = setTimeout(later, time); - } - return timerId; - }; -}; From 20f3fe07b3fdf76ba1c0f7ce245928cdab1844c2 Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Thu, 9 Nov 2017 15:31:06 -0800 Subject: [PATCH 7/8] Switch to function declarations --- src/util/throttle.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/throttle.js b/src/util/throttle.js index 15ffceee0eb..d2f7cb47a84 100644 --- a/src/util/throttle.js +++ b/src/util/throttle.js @@ -12,7 +12,7 @@ module.exports = function(unthrottledFunction: () => void, period: number): () = // function at a later time. `false` if there is not. let pending = false; - const throttledFunction = () => { + function throttledFunction() { const time = Date.now(); if (nextTime <= time && !pending) { @@ -23,14 +23,14 @@ module.exports = function(unthrottledFunction: () => void, period: number): () = // eslint-disable-next-line no-use-before-define setTimeout(_throttledFunction, nextTime - time); } - }; + } // This callback to `setTimeout` is written outside `throttledFunction` to // reduce the number of closures created. - const _throttledFunction = () => { + function _throttledFunction() { pending = false; throttledFunction(); - }; + } return throttledFunction; }; From 6eb096bce1ff2035dcbae15a6400f1d0af6d5e2b Mon Sep 17 00:00:00 2001 From: Lucas Wojciechowski Date: Thu, 9 Nov 2017 16:27:22 -0800 Subject: [PATCH 8/8] Use @mourner's throttle implementation --- src/ui/hash.js | 7 +++--- src/util/throttle.js | 42 +++++++++++++-------------------- test/unit/ui/hash.test.js | 4 +++- test/unit/util/throttle.test.js | 12 ++++++---- 4 files changed, 30 insertions(+), 35 deletions(-) diff --git a/src/ui/hash.js b/src/ui/hash.js index 82f026de2d3..28e97dabfa6 100644 --- a/src/ui/hash.js +++ b/src/ui/hash.js @@ -14,17 +14,16 @@ import type Map from './map'; */ class Hash { _map: Map; - _updateHash: () => void; + _updateHash: () => number; - constructor(options: ?{throttle?: Boolean;}) { + constructor() { util.bindAll([ '_onHashChange', '_updateHash' ], this); // Mobile Safari doesn't allow updating the hash more than 100 times per 30 seconds. - const throttleTime = (options && options.throttle === false) ? 0 : 30 * 1000 / 100; - this._updateHash = throttle(this._updateHashUnthrottled.bind(this), throttleTime); + this._updateHash = throttle(this._updateHashUnthrottled.bind(this), 30 * 1000 / 100); } /* diff --git a/src/util/throttle.js b/src/util/throttle.js index d2f7cb47a84..ba5ec051738 100644 --- a/src/util/throttle.js +++ b/src/util/throttle.js @@ -3,34 +3,24 @@ /** * Throttle the given function to run at most every `period` milliseconds. */ -module.exports = function(unthrottledFunction: () => void, period: number): () => void { - - // The next time (unix epoch) that the function is allowed to execute - let nextTime = 0; - - // `true` if there is a pending "setTimeout" operation that'll invoke the - // function at a later time. `false` if there is not. +module.exports = function throttle(fn: () => void, time: number): () => number { let pending = false; + let timerId = 0; - function throttledFunction() { - const time = Date.now(); - - if (nextTime <= time && !pending) { - nextTime = time + period; - unthrottledFunction(); - } else if (!pending) { - pending = true; - // eslint-disable-next-line no-use-before-define - setTimeout(_throttledFunction, nextTime - time); + const later = () => { + timerId = 0; + if (pending) { + fn(); + timerId = setTimeout(later, time); + pending = false; } - } + }; - // This callback to `setTimeout` is written outside `throttledFunction` to - // reduce the number of closures created. - function _throttledFunction() { - pending = false; - throttledFunction(); - } - - return throttledFunction; + return () => { + pending = true; + if (!timerId) { + later(); + } + return timerId; + }; }; diff --git a/test/unit/ui/hash.test.js b/test/unit/ui/hash.test.js index b7805added4..67f7fa7b99d 100644 --- a/test/unit/ui/hash.test.js +++ b/test/unit/ui/hash.test.js @@ -7,7 +7,9 @@ const Map = require('../../../src/ui/map'); test('hash', (t) => { function createHash() { - return new Hash({throttle: false}); + const hash = new Hash(); + hash._updateHash = hash._updateHashUnthrottled.bind(hash); + return hash; } function createMap() { diff --git a/test/unit/util/throttle.test.js b/test/unit/util/throttle.test.js index 11a684c3f8c..962d89ba9da 100644 --- a/test/unit/util/throttle.test.js +++ b/test/unit/util/throttle.test.js @@ -13,14 +13,18 @@ test('throttle', (t) => { t.end(); }); - t.test('executes unthrottled function immediately when period is 0', (t) => { + t.test('executes unthrottled function once per tick when period is 0', (t) => { let executionCount = 0; const throttledFunction = throttle(() => { executionCount++; }, 0); throttledFunction(); throttledFunction(); - throttledFunction(); - t.equal(executionCount, 3); - t.end(); + t.equal(executionCount, 1); + setTimeout(() => { + throttledFunction(); + throttledFunction(); + t.equal(executionCount, 2); + t.end(); + }, 0); }); t.test('executes unthrottled function immediately once when period is > 0', (t) => {