From de53b0488262a9fb6ee790047751f89c8858afe6 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 5 Aug 2019 13:25:05 +0200 Subject: [PATCH 1/5] Added the `Watchdog#_now()` method. --- src/watchdog.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/watchdog.js b/src/watchdog.js index f0983c7..b61eef1 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -64,6 +64,14 @@ export default class Watchdog { */ this._crashNumberLimit = typeof config.crashNumberLimit === 'number' ? config.crashNumberLimit : 3; + /** + * Returns the result of `Date.now()` call. It can be overridden in tests to mock time as the popular + * approaches like `sinon.useFakeTimers()` does not work well with error handling. + * + * @protected + */ + this._now = () => Date.now(); + /** * @private * @type {Number} @@ -311,7 +319,7 @@ export default class Watchdog { source: evt.source, lineno: evt.lineno, colno: evt.colno, - date: Date.now() + date: this._now() } ); this.fire( 'error', { error: evt.error } ); From 2ba91c983e54adc0748b26b83a64aa0cf75a64ea Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 5 Aug 2019 22:56:25 +0200 Subject: [PATCH 2/5] Moved throttled data saving to the `destory()` method. --- src/watchdog.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/watchdog.js b/src/watchdog.js index b61eef1..ecdffa9 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -266,6 +266,9 @@ export default class Watchdog { window.removeEventListener( 'error', this._boundErrorHandler ); this.stopListening( this._editor.model.document, 'change:data', this._throttledSave ); + // Save data if there are remaining changes. + this._throttledSave.flush(); + return Promise.resolve() .then( () => this._destructor( this._editor ) ) .then( () => { @@ -382,7 +385,6 @@ export default class Watchdog { */ _restart() { this.state = 'initializing'; - this._throttledSave.flush(); return Promise.resolve() .then( () => this._destroy() ) From 20ed93456fd225a1faf3b0fbc2d28d82b3825b2d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 6 Aug 2019 12:09:03 +0200 Subject: [PATCH 3/5] Added test for the `Watchdog#destroy()` method. --- src/watchdog.js | 9 +++++++-- tests/watchdog.js | 42 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/watchdog.js b/src/watchdog.js index ecdffa9..c4b29cb 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -95,7 +95,10 @@ export default class Watchdog { * @private * @type {Function} */ - this._throttledSave = throttle( this._save.bind( this ), config.saveInterval || 5000 ); + this._throttledSave = throttle( + this._save.bind( this ), + typeof config.saveInterval === 'number' ? config.saveInterval : 5000 + ); /** * The current editor instance. @@ -266,7 +269,7 @@ export default class Watchdog { window.removeEventListener( 'error', this._boundErrorHandler ); this.stopListening( this._editor.model.document, 'change:data', this._throttledSave ); - // Save data if there are remaining changes. + // Save data if there is a remaining editor data change. this._throttledSave.flush(); return Promise.resolve() @@ -285,6 +288,8 @@ export default class Watchdog { _save() { const version = this._editor.model.document.version; + console.log( 'saving' ); + // Change may not produce an operation, so the document's version // can be the same after that change. if ( version === this._lastDocumentVersion ) { diff --git a/tests/watchdog.js b/tests/watchdog.js index c5d80d5..72a2e0e 100644 --- a/tests/watchdog.js +++ b/tests/watchdog.js @@ -369,7 +369,7 @@ describe( 'Watchdog', () => { } ); it( 'Watchdog should crash permanently if the `crashNumberLimit` is reached' + - ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (default values)', () => { + ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (default values)', () => { const watchdog = new Watchdog(); watchdog.setCreator( ( el, config ) => ClassicTestEditor.create( el, config ) ); @@ -406,7 +406,7 @@ describe( 'Watchdog', () => { } ); it( 'Watchdog should crash permanently if the `crashNumberLimit` is reached' + - ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (custom values)', () => { + ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (custom values)', () => { const watchdog = new Watchdog( { crashNumberLimit: 2, minimumNonErrorTimePeriod: 1000 } ); watchdog.setCreator( ( el, config ) => ClassicTestEditor.create( el, config ) ); @@ -695,6 +695,44 @@ describe( 'Watchdog', () => { } ); } ); + describe( 'destroy()', () => { + // See #19. + it( 'should clean internal stuff', () => { + // 30ms should be enough to make the two data changes split into two data save actions. + // This will ensure that the second data save action will be put off in time. + const SAVE_INTERVAL = 30; + + const watchdog = Watchdog.for( ClassicTestEditor, { + saveInterval: SAVE_INTERVAL, + } ); + + return watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ).then( () => { + const doc = watchdog.editor.model.document; + + watchdog.editor.model.change( writer => { + writer.insertText( 'bar', writer.createPositionAt( doc.getRoot(), 1 ) ); + } ); + + watchdog.editor.model.change( writer => { + writer.insertText( 'foo', writer.createPositionAt( doc.getRoot(), 1 ) ); + } ); + } ).then( () => { + return watchdog.destroy(); + } ).then( () => { + // Wait to ensure that the throttled save is cleared and won't be executed + // on the non-existing editor. + return new Promise( res => setTimeout( res, SAVE_INTERVAL ) ); + } ).then( () => { + expect( watchdog.editor ).to.equal( null ); + expect( watchdog.state ).to.equal( 'destroyed' ); + expect( watchdog.crashes ).to.deep.equal( [] ); + } ); + } ); + } ); + describe( 'static for()', () => { it( 'should be a shortcut method for creating the watchdog', () => { const watchdog = Watchdog.for( ClassicTestEditor ); From b259298af6feaca82a005c613eb7898d140d16ba Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 6 Aug 2019 12:09:41 +0200 Subject: [PATCH 4/5] Removed console.log(). --- src/watchdog.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/watchdog.js b/src/watchdog.js index c4b29cb..3ca2e88 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -288,8 +288,6 @@ export default class Watchdog { _save() { const version = this._editor.model.document.version; - console.log( 'saving' ); - // Change may not produce an operation, so the document's version // can be the same after that change. if ( version === this._lastDocumentVersion ) { From 641aeb10f4af5ec089ff80a55788cc4a31921c45 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 11:02:47 +0200 Subject: [PATCH 5/5] Simplified Date.now() Co-Authored-By: Piotr Jasiun --- src/watchdog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/watchdog.js b/src/watchdog.js index 3ca2e88..e5abe4a 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -70,7 +70,7 @@ export default class Watchdog { * * @protected */ - this._now = () => Date.now(); + this._now = Date.now; /** * @private