From 6fe6e03a48e401a126202fd9b49366b3097c8eaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 16 Jan 2019 14:41:38 +0100 Subject: [PATCH 1/4] Add catch block for failed file promise in `FileRepository`. --- src/filerepository.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/filerepository.js b/src/filerepository.js index c4fb8c0..b7bbdf9 100644 --- a/src/filerepository.js +++ b/src/filerepository.js @@ -183,9 +183,13 @@ export default class FileRepository extends Plugin { // Store also file => loader mapping so loader can be retrieved by file instance returned upon Promise resolution. if ( fileOrPromise instanceof Promise ) { - loader.file.then( file => { - this._loadersMap.set( file, loader ); - } ); + loader.file + .then( file => { + this._loadersMap.set( file, loader ); + } ).catch( () => { + // There was an error fetching file, so do not add anything to `this._loadersMap`. + // Also the error will be handled by `FileLoader` so no action is required here. + } ); } loader.on( 'change:uploaded', () => { From 26a817a4aaa20497fe0d28e7dbb9a129274e8622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 22 Jan 2019 15:13:16 +0100 Subject: [PATCH 2/4] Always catch failed file promise. --- src/filerepository.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/filerepository.js b/src/filerepository.js index b7bbdf9..3335af7 100644 --- a/src/filerepository.js +++ b/src/filerepository.js @@ -183,15 +183,17 @@ export default class FileRepository extends Plugin { // Store also file => loader mapping so loader can be retrieved by file instance returned upon Promise resolution. if ( fileOrPromise instanceof Promise ) { - loader.file - .then( file => { - this._loadersMap.set( file, loader ); - } ).catch( () => { - // There was an error fetching file, so do not add anything to `this._loadersMap`. - // Also the error will be handled by `FileLoader` so no action is required here. - } ); + loader.file.then( file => { + this._loadersMap.set( file, loader ); + } ); } + // Catch the file promise rejection. If there are no `catch` closures, the browser + // will throw an error (see https://github.com/ckeditor/ckeditor5-upload/pull/90). + loader.file.catch( () => { + // The error will be handled by `FileLoader` so no action is required here. + } ); + loader.on( 'change:uploaded', () => { let aggregatedUploaded = 0; From 5bdcae4bb05250cb75f2e2508ad8532d5c1b703b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 22 Jan 2019 15:13:56 +0100 Subject: [PATCH 3/4] Tests: Check if file promise always has a `catch` clause. --- tests/filerepository.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/filerepository.js b/tests/filerepository.js index 49f6a52..44abbb1 100644 --- a/tests/filerepository.js +++ b/tests/filerepository.js @@ -200,6 +200,24 @@ describe( 'FileRepository', () => { expect( fileRepository.uploadTotal ).to.equal( 500 ); expect( fileRepository.uploadedPercent ).to.equal( 20 ); } ); + + it( 'should catch if file promise rejected (file)', () => { + const catchSpy = testUtils.sinon.spy( Promise.prototype, 'catch' ); + + fileRepository.createLoader( createNativeFileMock() ); + + // One call originates from `loader._createFilePromiseWrapper()` and other from `fileRepository.createLoader()`. + expect( catchSpy.callCount ).to.equal( 2 ); + } ); + + it( 'should catch if file promise rejected (promise)', () => { + const catchSpy = testUtils.sinon.spy( Promise.prototype, 'catch' ); + + fileRepository.createLoader( new Promise( () => {} ) ); + + // One call originates from `loader._createFilePromiseWrapper()` and other from `fileRepository.createLoader()`. + expect( catchSpy.callCount ).to.equal( 2 ); + } ); } ); describe( 'getLoader()', () => { From 83b78bd4ad55b473c94dd7fea8d1fcdb72723583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 22 Jan 2019 15:14:43 +0100 Subject: [PATCH 4/4] Rewording. --- src/filerepository.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/filerepository.js b/src/filerepository.js index 3335af7..396939d 100644 --- a/src/filerepository.js +++ b/src/filerepository.js @@ -188,7 +188,7 @@ export default class FileRepository extends Plugin { } ); } - // Catch the file promise rejection. If there are no `catch` closures, the browser + // Catch the file promise rejection. If there are no `catch` clause, the browser // will throw an error (see https://github.com/ckeditor/ckeditor5-upload/pull/90). loader.file.catch( () => { // The error will be handled by `FileLoader` so no action is required here.