From 6aabe915e891bbbac5cc52658136d87ef21eddfd Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 21 Dec 2021 14:45:09 -0500 Subject: [PATCH 1/9] Added tasts --- .vscode/tasks.json | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 .vscode/tasks.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json new file mode 100644 index 000000000000..ea550d21e7a9 --- /dev/null +++ b/.vscode/tasks.json @@ -0,0 +1,70 @@ +{ + "version": "2.0.0", + "tasks": [ + { + "label": "amp: Run unit test in this file", + "detail": "amp unit --files=${file} --headless --verbose", + "type": "shell", + "command": "amp", + "args": ["unit", "--files=${file}", "--headless", "--verbose"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + } + }, + { + "label": "amp: Watch unit test in this file", + "detail": "amp unit --files=${file} --headless --verbose --watch", + "type": "shell", + "command": "amp", + "args": ["unit", "--files=${file}", "--headless", "--verbose", "--watch"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + }, + "isBackground": true + }, + { + "label": "amp: Run unit tests in all changed files", + "detail": "amp unit --local_changes --headless --verbose", + "type": "shell", + "command": "amp", + "args": ["unit", "--local_changes", "--headless", "--verbose"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + } + }, + { + "label": "amp: Check PR", + "detail": "amp pr-check --nobuild", + "type": "shell", + "command": "amp", + "args": ["pr-check", "--nobuild"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + } + }, + ] +} \ No newline at end of file From dc1fa876f8e029a54a7cfd4bf2f70795da071102 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 21 Dec 2021 14:47:32 -0500 Subject: [PATCH 2/9] Undo --- .vscode/tasks.json | 70 ---------------------------------------------- 1 file changed, 70 deletions(-) delete mode 100644 .vscode/tasks.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json deleted file mode 100644 index ea550d21e7a9..000000000000 --- a/.vscode/tasks.json +++ /dev/null @@ -1,70 +0,0 @@ -{ - "version": "2.0.0", - "tasks": [ - { - "label": "amp: Run unit test in this file", - "detail": "amp unit --files=${file} --headless --verbose", - "type": "shell", - "command": "amp", - "args": ["unit", "--files=${file}", "--headless", "--verbose"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - } - }, - { - "label": "amp: Watch unit test in this file", - "detail": "amp unit --files=${file} --headless --verbose --watch", - "type": "shell", - "command": "amp", - "args": ["unit", "--files=${file}", "--headless", "--verbose", "--watch"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - }, - "isBackground": true - }, - { - "label": "amp: Run unit tests in all changed files", - "detail": "amp unit --local_changes --headless --verbose", - "type": "shell", - "command": "amp", - "args": ["unit", "--local_changes", "--headless", "--verbose"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - } - }, - { - "label": "amp: Check PR", - "detail": "amp pr-check --nobuild", - "type": "shell", - "command": "amp", - "args": ["pr-check", "--nobuild"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - } - }, - ] -} \ No newline at end of file From 974e219dc69f557064b65a3b18ce414186fcdfc1 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 11:40:14 -0400 Subject: [PATCH 3/9] Make observable work with removing items while firing --- src/core/data-structures/observable.js | 18 +++++++++++++++++- .../core/data-structures/test-observable.js | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/core/data-structures/observable.js b/src/core/data-structures/observable.js index dd8390749b5e..73fc26c89a7c 100644 --- a/src/core/data-structures/observable.js +++ b/src/core/data-structures/observable.js @@ -12,6 +12,12 @@ export class Observable { constructor() { /** @type {?Array} */ this.handlers_ = null; + + /** @type {!Array} */ + this.handlersToRemove_ = []; + + /** @type {boolean} */ + this.iterating_ = false; } /** @@ -31,13 +37,18 @@ export class Observable { /** * Removes the observer from this instance. + * Can be called in a handler fired. * @param {function(TYPE=):void} handler Observer's instance. */ remove(handler) { if (!this.handlers_) { return; } - removeItem(this.handlers_, handler); + if (this.iterating_) { + this.handlersToRemove_.push(handler); + } else { + removeItem(this.handlers_, handler); + } } /** @@ -58,9 +69,14 @@ export class Observable { if (!this.handlers_) { return; } + this.iterating_ = true; for (const handler of this.handlers_) { handler(opt_event); } + this.iterating_ = false; + for (const handler of this.handlersToRemove_) { + removeItem(this.handlers_, handler); + } } /** diff --git a/test/unit/core/data-structures/test-observable.js b/test/unit/core/data-structures/test-observable.js index 22d7cadfcc52..5d3efe93f427 100644 --- a/test/unit/core/data-structures/test-observable.js +++ b/test/unit/core/data-structures/test-observable.js @@ -44,4 +44,20 @@ describes.sandboxed('data structures - Observable', {}, () => { expect(observer1Called).to.equal(1); expect(observer2Called).to.equal(2); }); + + it('remove while firing', () => { + let observer1Called = 0; + const observer1 = () => { + observer1Called++; + }; + observable.add(observer1); + const remove = observable.add(() => { + remove(); + }); + observable.add(observer1); + + observable.fire(); + + expect(observer1Called).to.equal(2); + }); }); From e0a87a15cd5e649133f36d72e4989b9215447661 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 11:46:59 -0400 Subject: [PATCH 4/9] Using set for removing --- src/core/data-structures/observable.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/core/data-structures/observable.js b/src/core/data-structures/observable.js index 73fc26c89a7c..80d915e8530e 100644 --- a/src/core/data-structures/observable.js +++ b/src/core/data-structures/observable.js @@ -1,4 +1,4 @@ -import {removeItem} from '#core/types/array'; +import {remove, removeItem} from '#core/types/array'; /** * This class helps to manage observers. Observers can be added, removed or @@ -13,8 +13,8 @@ export class Observable { /** @type {?Array} */ this.handlers_ = null; - /** @type {!Array} */ - this.handlersToRemove_ = []; + /** @type {!Set} */ + this.handlersToRemove_ = new Set(); /** @type {boolean} */ this.iterating_ = false; @@ -45,7 +45,7 @@ export class Observable { return; } if (this.iterating_) { - this.handlersToRemove_.push(handler); + this.handlersToRemove_.add(handler); } else { removeItem(this.handlers_, handler); } @@ -77,6 +77,10 @@ export class Observable { for (const handler of this.handlersToRemove_) { removeItem(this.handlers_, handler); } + if (this.handlersToRemove_.size) { + remove(this.handlers_, (handler) => this.handlersToRemove_.has(handler)); + this.handlersToRemove_.clear(); + } } /** From 5dc4e9341887aa2483be275b97dcaabb3cbc30e7 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 12:44:39 -0400 Subject: [PATCH 5/9] Remove previous code --- src/core/data-structures/observable.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/core/data-structures/observable.js b/src/core/data-structures/observable.js index 80d915e8530e..930d18698145 100644 --- a/src/core/data-structures/observable.js +++ b/src/core/data-structures/observable.js @@ -74,9 +74,6 @@ export class Observable { handler(opt_event); } this.iterating_ = false; - for (const handler of this.handlersToRemove_) { - removeItem(this.handlers_, handler); - } if (this.handlersToRemove_.size) { remove(this.handlers_, (handler) => this.handlersToRemove_.has(handler)); this.handlersToRemove_.clear(); From c5d9d63973f9fcc5124fa1306dbcfb9a818dd688 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 13:09:21 -0400 Subject: [PATCH 6/9] Added iterating depth --- src/core/data-structures/observable.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/data-structures/observable.js b/src/core/data-structures/observable.js index 930d18698145..18e869e89935 100644 --- a/src/core/data-structures/observable.js +++ b/src/core/data-structures/observable.js @@ -16,8 +16,8 @@ export class Observable { /** @type {!Set} */ this.handlersToRemove_ = new Set(); - /** @type {boolean} */ - this.iterating_ = false; + /** @type {number} */ + this.iteratingDepth_ = 0; } /** @@ -44,7 +44,7 @@ export class Observable { if (!this.handlers_) { return; } - if (this.iterating_) { + if (this.iteratingDepth_ > 0) { this.handlersToRemove_.add(handler); } else { removeItem(this.handlers_, handler); @@ -69,12 +69,12 @@ export class Observable { if (!this.handlers_) { return; } - this.iterating_ = true; + this.iteratingDepth_++; for (const handler of this.handlers_) { handler(opt_event); } - this.iterating_ = false; - if (this.handlersToRemove_.size) { + this.iteratingDepth_--; + if (this.handlersToRemove_.size && this.iteratingDepth_ == 0) { remove(this.handlers_, (handler) => this.handlersToRemove_.has(handler)); this.handlersToRemove_.clear(); } From e844875ebacd965b6b39cd9476cb9bcce6b3388a Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 13:49:58 -0400 Subject: [PATCH 7/9] Added test to check iterations of handler removal --- .../core/data-structures/test-observable.js | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/unit/core/data-structures/test-observable.js b/test/unit/core/data-structures/test-observable.js index 5d3efe93f427..d898fa51effb 100644 --- a/test/unit/core/data-structures/test-observable.js +++ b/test/unit/core/data-structures/test-observable.js @@ -60,4 +60,32 @@ describes.sandboxed('data structures - Observable', {}, () => { expect(observer1Called).to.equal(2); }); + + it('remove while firing on multiple depths', () => { + // First depth of iteration will remove the handler, should not stop second depth. + let observer1Called = 0; + let iterations = 0; + const observer1 = () => { + observer1Called++; + }; + const secondRemove = observable.add(observer1); + observable.add(() => { + iterations++; + if (iterations == 1) { + secondRemove(); + observable.fire(); + } + }); + + // At this point handlers are: [increaseByOne, removePreviousHandler] + // When firing the first time, handler should not be removed until the end. + observable.fire(); + expect(observer1Called).to.equal(2); + expect(iterations).to.equal(2); + + // When firing again, handler should have been removed. + observable.fire(); + expect(observer1Called).to.equal(2); + expect(iterations).to.equal(3); + }); }); From 09809c6af4e2265dc2435a667839ee3131ac8d38 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 16:00:56 -0400 Subject: [PATCH 8/9] Simplified impl and cleaned unused tests --- src/core/data-structures/observable.js | 23 ++------------- .../core/data-structures/test-observable.js | 28 ------------------- 2 files changed, 3 insertions(+), 48 deletions(-) diff --git a/src/core/data-structures/observable.js b/src/core/data-structures/observable.js index 18e869e89935..52152fcee032 100644 --- a/src/core/data-structures/observable.js +++ b/src/core/data-structures/observable.js @@ -1,4 +1,4 @@ -import {remove, removeItem} from '#core/types/array'; +import {removeItem} from '#core/types/array'; /** * This class helps to manage observers. Observers can be added, removed or @@ -12,12 +12,6 @@ export class Observable { constructor() { /** @type {?Array} */ this.handlers_ = null; - - /** @type {!Set} */ - this.handlersToRemove_ = new Set(); - - /** @type {number} */ - this.iteratingDepth_ = 0; } /** @@ -37,18 +31,13 @@ export class Observable { /** * Removes the observer from this instance. - * Can be called in a handler fired. * @param {function(TYPE=):void} handler Observer's instance. */ remove(handler) { if (!this.handlers_) { return; } - if (this.iteratingDepth_ > 0) { - this.handlersToRemove_.add(handler); - } else { - removeItem(this.handlers_, handler); - } + removeItem(this.handlers_, handler); } /** @@ -69,15 +58,9 @@ export class Observable { if (!this.handlers_) { return; } - this.iteratingDepth_++; - for (const handler of this.handlers_) { + for (const handler of this.handlers_.slice()) { handler(opt_event); } - this.iteratingDepth_--; - if (this.handlersToRemove_.size && this.iteratingDepth_ == 0) { - remove(this.handlers_, (handler) => this.handlersToRemove_.has(handler)); - this.handlersToRemove_.clear(); - } } /** diff --git a/test/unit/core/data-structures/test-observable.js b/test/unit/core/data-structures/test-observable.js index d898fa51effb..5d3efe93f427 100644 --- a/test/unit/core/data-structures/test-observable.js +++ b/test/unit/core/data-structures/test-observable.js @@ -60,32 +60,4 @@ describes.sandboxed('data structures - Observable', {}, () => { expect(observer1Called).to.equal(2); }); - - it('remove while firing on multiple depths', () => { - // First depth of iteration will remove the handler, should not stop second depth. - let observer1Called = 0; - let iterations = 0; - const observer1 = () => { - observer1Called++; - }; - const secondRemove = observable.add(observer1); - observable.add(() => { - iterations++; - if (iterations == 1) { - secondRemove(); - observable.fire(); - } - }); - - // At this point handlers are: [increaseByOne, removePreviousHandler] - // When firing the first time, handler should not be removed until the end. - observable.fire(); - expect(observer1Called).to.equal(2); - expect(iterations).to.equal(2); - - // When firing again, handler should have been removed. - observable.fire(); - expect(observer1Called).to.equal(2); - expect(iterations).to.equal(3); - }); }); From be19f25da8b672e5485bb6e2b3298806cb61ed83 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 17 Mar 2022 17:11:40 -0400 Subject: [PATCH 9/9] Added comment --- src/core/data-structures/observable.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/data-structures/observable.js b/src/core/data-structures/observable.js index 52152fcee032..b2cd5a0c1a9b 100644 --- a/src/core/data-structures/observable.js +++ b/src/core/data-structures/observable.js @@ -58,6 +58,7 @@ export class Observable { if (!this.handlers_) { return; } + // Iterate over copy of handlers_ in case handlers are removed inside. for (const handler of this.handlers_.slice()) { handler(opt_event); }