amp-bind: Rate-limit history operations#23938
amp-bind: Rate-limit history operations#23938dreamofabear merged 11 commits intoampproject:masterfrom
Conversation
f991d7d to
c2a7d17
Compare
7558fed to
093abb8
Compare
|
/to @jridgewell |
| */ | ||
| this.historyQueue_ = []; | ||
|
|
||
| /** @private @const {!Function} */ |
There was a problem hiding this comment.
| /** @private @const {!Function} */ | |
| /** @private @const {function()} */ |
| this.debouncedFlushHistoryQueue_ = debounce( | ||
| this.win_, | ||
| () => this.flushHistoryQueue_(), | ||
| 1000 |
There was a problem hiding this comment.
Depending on how quickly these calls are coming in, we may never replace the history. Eg, if a slidechange happens every .5s, this debounce will never trigger (it's waiting for 1s to pass since the last call before invoking).
There was a problem hiding this comment.
True, but I think this is not a big deal in practice since it only affects users that close the page very quickly after a setState action.
extensions/amp-bind/0.1/bind-impl.js
Outdated
| this.historyQueue_.push({op: HistoryOp.PUSH, data, onPop}); | ||
| // Immediately flush queue on "push" to avoid adding latency to | ||
| // the browser "back" functionality. | ||
| this.flushHistoryQueue_(); |
There was a problem hiding this comment.
We should add a way to cancel the debounced timer, if any is pending.
There was a problem hiding this comment.
I don't think that's worth the extra code since flushing an empty queue is harmless.
extensions/amp-bind/0.1/bind-impl.js
Outdated
| // Skip consecutive "replace" operations. | ||
| const next = this.historyQueue_[i + 1]; | ||
| if (next && next.op === HistoryOp.REPLACE) { | ||
| continue; |
There was a problem hiding this comment.
It seems like this could be done better at the push side, which'll prevent this queue from getting massive.
const last = getLast(this.historyQueue_);
if (last && last.op === HistoryOp.REPLACE) {
last.data = data;
} else {
this.historyQueue_.push({ op: HistoryOp.REPLACE, data });
}
extensions/amp-bind/0.1/bind-impl.js
Outdated
| // may contain user data e.g. form input. | ||
| return this.viewer_.isTrustedViewer().then(trusted => { | ||
| return trusted ? data : null; | ||
| return trusted ? getData() : undefined; |
There was a problem hiding this comment.
Defering the copy like this creates a slight change of behavior, since we'll now copy the data after isTrustedViewer resolves instead of before.
There was a problem hiding this comment.
Yea, this is probably an unnecessary optimization since it only affects untrusted viewers. Removed.
extensions/amp-bind/0.1/bind-impl.js
Outdated
| // Collapse if consecutive "replace" operations. | ||
| if (task.op === HistoryOp.REPLACE) { | ||
| const lastTask = this.historyQueue_[this.historyQueue_.length - 1]; | ||
| if (lastTask && lastTask.op === HistoryOp.REPLACE) { |
There was a problem hiding this comment.
Nit: Check that there's at least 1 queued item before, instead of doing a missed lookup.
…cript-img-with-http-protocol * 'master' of github.com:ampproject/amphtml: (1326 commits) Fix and enable e2e tests for AMPHTML ads FIE rendering mode (ampproject#23995) 🏗 Update WorkerDOM to 0.17.0 (ampproject#24024) Make DocInfo.pageViewId64 async (ampproject#23998) 🐛 Updates amp-sidebar in amp-story (ampproject#23956) Revert "Revert "📖Update documentation for carousel 0.2 (ampproject#23840)" (ampproject#23967)" (ampproject#24016) 🔥 Revert "📈 Initial StorySpec Implementation (ampproject#23030)" (ampproject#24013) Extension skeleton code for payment widgets (ampproject#23045) 🏗🐛 Don't call `travisBuildNumber()` in the global scope (ampproject#24021) Remove suppressTypes from amp-mustache. (ampproject#23993) 🐛 Move `terser` from `dependencies` to `devDependencies` (ampproject#24018) Revert "Revert "Set the new loaders experiment to 1% of traffic. (ampproject#23780)" (ampproject#23963)" (ampproject#24014) SwG release 0.1.22.63 (ampproject#23997) Resolve navTiming variable earlier if possible (ampproject#23580) 🏗 Don't run all the runtime tests for validator-only changes (ampproject#24010) Collect document ready signal (ampproject#23981) Validator rollup (ampproject#24000) Remove flaky story branching test. (ampproject#23994) Include amp-base-carousel in amp-carousel's build. (ampproject#23984) Partial validator rollup (ampproject#23996) amp-bind: Rate-limit history operations (ampproject#23938) ...
This reverts commit fb19bc7.
Fixes #17969.