8
0
Krzysztof Krztoń 7 سال پیش
والد
کامیت
dfcb4983d1
2فایلهای تغییر یافته به همراه111 افزوده شده و 127 حذف شده
  1. 36 43
      packages/ckeditor5-upload/src/filerepository.js
  2. 75 84
      packages/ckeditor5-upload/tests/filerepository.js

+ 36 - 43
packages/ckeditor5-upload/src/filerepository.js

@@ -65,7 +65,13 @@ export default class FileRepository extends Plugin {
 		this.loaders.on( 'add', () => this._updatePendingAction() );
 		this.loaders.on( 'remove', () => this._updatePendingAction() );
 
-		this.loadersMap = new Map();
+		/**
+		 * Loaders mappings used to retrieve loaders references.
+		 *
+		 * @private
+		 * @member {Map<File|Promise, FileLoader>} #_loadersMap
+		 */
+		this._loadersMap = new Map();
 
 		/**
 		 * Reference to a pending action registered in a {@link module:core/pendingactions~PendingActions} plugin
@@ -131,21 +137,7 @@ export default class FileRepository extends Plugin {
 	 * @returns {module:upload/filerepository~FileLoader|null}
 	 */
 	getLoader( fileOrPromise ) {
-		// When `createLoader()` is called with `File` instance, the file is used in a map. When it is called with `Promise`
-		// instance, the promise is used in a map. So here it retrieves loader based on the same object it was initialized with.
-		const loaderInstance = this.loadersMap.get( fileOrPromise );
-
-		// If `createLoader()` is called with `Promise` instance, once the promise is resolved, the `loader.file` returns
-		// a file instance which can be also used here (and is not in the `loadersMap`). The code below covers this case.
-		if ( !loaderInstance && !( fileOrPromise instanceof Promise ) ) {
-			for ( const loader of this.loaders ) {
-				if ( loader.file === fileOrPromise ) {
-					return loader;
-				}
-			}
-		}
-
-		return loaderInstance || null;
+		return this._loadersMap.get( fileOrPromise ) || null;
 	}
 
 	/**
@@ -199,7 +191,14 @@ export default class FileRepository extends Plugin {
 		loader._adapter = this.createUploadAdapter( loader );
 
 		this.loaders.add( loader );
-		this.loadersMap.set( fileOrPromise, loader );
+		this._loadersMap.set( fileOrPromise, loader );
+
+		// 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.on( 'change:uploaded', () => {
 			let aggregatedUploaded = 0;
@@ -239,17 +238,11 @@ export default class FileRepository extends Plugin {
 
 		this.loaders.remove( loader );
 
-		if ( !( fileOrPromiseOrLoader instanceof FileLoader ) ) {
-			// If File or Promise instance is passed, it is the key in map and can be simply use to remove map entry.
-			this.loadersMap.delete( fileOrPromiseOrLoader );
-		} else {
-			// If FileLoader instance is passed, iterate over map entries to find its key and remove whole entry.
-			this.loadersMap.forEach( ( value, key ) => {
-				if ( value === fileOrPromiseOrLoader ) {
-					this.loadersMap.delete( key );
-				}
-			} );
-		}
+		this._loadersMap.forEach( ( value, key ) => {
+			if ( value === loader ) {
+				this._loadersMap.delete( key );
+			}
+		} );
 	}
 
 	/**
@@ -298,14 +291,6 @@ class FileLoader {
 		 */
 		this.id = uid();
 
-		/**
-		 * A `File` instance associated with this file loader.
-		 *
-		 * @private
-		 * @member {File}
-		 */
-		this._file = null;
-
 		/**
 		 * Additional wrapper over a file promise passed to this loader.
 		 *
@@ -398,12 +383,22 @@ class FileLoader {
 	}
 
 	/**
-	 * Returns a `File` instance associated with this file loader.
+	 * Returns a `Promise` which resolves to a `File` instance associated with this file loader.
 	 *
-	 * @type {File|null}
+	 * @type {Promise}
 	 */
 	get file() {
-		return this._file;
+		if ( !this._filePromiseWrapper ) {
+			// Loader was destroyed, return promise which resolves to null.
+			return new Promise( resolve => resolve( null ) );
+		} else {
+			// The `loader.file.then( ... )` can be called, then `loader._destroy()` (which is sync) and then
+			// `loader.file.then( ... )` will be resolved. In such case, even after `destroy` call the promise will
+			// return `File` instance so we use nested promises which handles this case.
+			return this._filePromiseWrapper.promise.then( file => {
+				return new Promise( resolve => resolve( this._filePromiseWrapper ? file : null ) );
+			} );
+		}
 	}
 
 	/**
@@ -521,7 +516,6 @@ class FileLoader {
 	 * @private
 	 */
 	_destroy() {
-		this._file = undefined;
 		this._filePromiseWrapper = undefined;
 		this._reader = undefined;
 		this._adapter = undefined;
@@ -530,8 +524,8 @@ class FileLoader {
 	}
 
 	/**
-	 * Wraps a given file promise into another promise giving additional control (resolving, rejecting, checking if fulfilled)
-	 * over it. Additionally when file promise is resolved, its result (File instance) is assigned to this loader instance.
+	 * Wraps a given file promise into another promise giving additional
+	 * control (resolving, rejecting, checking if fulfilled) over it.
 	 *
 	 * @private
 	 * @param filePromise The file promise to be wrapped.
@@ -552,7 +546,6 @@ class FileLoader {
 			filePromise
 				.then( file => {
 					wrapper.isFulfilled = true;
-					this._file = file;
 					resolve( file );
 				} )
 				.catch( err => {

+ 75 - 84
packages/ckeditor5-upload/tests/filerepository.js

@@ -229,11 +229,10 @@ describe( 'FileRepository', () => {
 			const promise = new Promise( resolve => resolve( createNativeFileMock() ) );
 			const loader = fileRepository.createLoader( promise );
 
-			// Defer so file promise can be resolved.
-			setTimeout( () => {
-				expect( fileRepository.getLoader( loader.file ) ).to.equal( loader );
+			loader.file.then( fileInstance => {
+				expect( fileRepository.getLoader( fileInstance ) ).to.equal( loader );
 				done();
-			}, 0 );
+			} );
 		} );
 	} );
 
@@ -251,6 +250,8 @@ describe( 'FileRepository', () => {
 
 			sinon.assert.calledOnce( destroySpy );
 			expect( fileRepository.getLoader( file ) ).to.be.null;
+			expect( fileRepository.loaders.length ).to.equal( 0 );
+			expect( Array.from( fileRepository._loadersMap.keys ).length ).to.equal( 0 );
 		} );
 
 		it( 'should destroy loader by provided file (initialized with file)', () => {
@@ -258,34 +259,48 @@ describe( 'FileRepository', () => {
 
 			sinon.assert.calledOnce( destroySpy );
 			expect( fileRepository.getLoader( file ) ).to.be.null;
+			expect( fileRepository.loaders.length ).to.equal( 0 );
+			expect( Array.from( fileRepository._loadersMap.keys ).length ).to.equal( 0 );
 		} );
 
 		it( 'should destroy loader by provided promise (initialized with promise)', () => {
+			fileRepository.destroyLoader( loader );
+
 			const promise = new Promise( resolve => resolve( createNativeFileMock() ) );
-			const loader = fileRepository.createLoader( promise );
+			const newLoader = fileRepository.createLoader( promise );
 
-			destroySpy = testUtils.sinon.spy( loader, '_destroy' );
+			destroySpy = testUtils.sinon.spy( newLoader, '_destroy' );
 
 			fileRepository.destroyLoader( promise );
 
 			sinon.assert.calledOnce( destroySpy );
 			expect( fileRepository.getLoader( promise ) ).to.be.null;
+			expect( fileRepository.loaders.length ).to.equal( 0 );
+			expect( Array.from( fileRepository._loadersMap.keys() ).length ).to.equal( 0 );
 		} );
 
-		it( 'should return loader by file instance (initialized with promise)', done => {
+		it( 'should destroy loader by provided file (initialized with promise)', done => {
+			fileRepository.destroyLoader( loader );
+
 			const promise = new Promise( resolve => resolve( createNativeFileMock() ) );
-			const loader = fileRepository.createLoader( promise );
+			const newLoader = fileRepository.createLoader( promise );
 
-			destroySpy = testUtils.sinon.spy( loader, '_destroy' );
+			destroySpy = testUtils.sinon.spy( newLoader, '_destroy' );
 
-			// Defer so file promise can be resolved.
-			setTimeout( () => {
-				fileRepository.destroyLoader( promise );
+			newLoader.file.then( fileInstance => {
+				tryExpect( done, () => {
+					expect( fileRepository.loaders.length ).to.equal( 1 );
+					expect( Array.from( fileRepository._loadersMap.keys() ).length ).to.equal( 2 );
 
-				sinon.assert.calledOnce( destroySpy );
-				expect( fileRepository.getLoader( loader.file ) ).to.be.null;
-				done();
-			}, 0 );
+					fileRepository.destroyLoader( fileInstance );
+
+					sinon.assert.calledOnce( destroySpy );
+
+					expect( fileRepository.getLoader( fileInstance ) ).to.be.null;
+					expect( fileRepository.loaders.length ).to.equal( 0 );
+					expect( Array.from( fileRepository._loadersMap.keys() ).length ).to.equal( 0 );
+				} );
+			} );
 		} );
 	} );
 
@@ -308,10 +323,6 @@ describe( 'FileRepository', () => {
 				expect( loader.id ).to.be.a( 'string' );
 			} );
 
-			it( 'should initialize file with null', () => {
-				expect( loader._file ).to.be.null;
-			} );
-
 			it( 'should initialize filePromiseWrapper', () => {
 				expect( loader._filePromiseWrapper ).to.not.be.null;
 				expect( loader._filePromiseWrapper.promise ).to.be.instanceOf( Promise );
@@ -388,37 +399,48 @@ describe( 'FileRepository', () => {
 		} );
 
 		describe( 'file getter', () => {
-			it( 'should return null', () => {
-				expect( loader.file ).to.be.null;
+			it( 'should return promise', () => {
+				expect( loader.file ).to.instanceof( Promise );
 			} );
 
-			it( 'should return file instance after file promise is resolved', done => {
-				let resolveFile;
-
-				const promise = new Promise( resolve => {
-					resolveFile = resolve;
+			it( 'should return promise which resolves to a file', done => {
+				loader.file.then( fileInstance => {
+					tryExpect( done, () => {
+						expect( fileInstance ).to.equal( file );
+					} );
 				} );
+			} );
 
-				const loader = fileRepository.createLoader( promise );
-
-				resolveFile( createNativeFileMock() );
+			it( 'should return promise which resolves to null after loader is destroyed (destroy before)', done => {
+				loader._destroy();
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					expect( loader.file ).to.not.null;
-					done();
-				}, 0 );
+				loader.file.then( fileInstance => {
+					tryExpect( done, () => {
+						expect( fileInstance ).to.be.null;
+					} );
+				} );
 			} );
 
-			it( 'should return undefined after loader is destroyed', () => {
-				// Mock file instance.
-				loader._file = { name: 'file1.jpg', size: 1024 };
-
-				expect( loader.file ).to.instanceOf( Object );
+			it( 'should return promise which resolves to null after loader is destroyed (destroy after)', done => {
+				loader.file.then( fileInstance => {
+					tryExpect( done, () => {
+						expect( fileInstance ).to.be.null;
+					} );
+				} );
 
 				loader._destroy();
+			} );
+
+			it( 'should return promise which resolves to null after loader is destroyed (file promise resolved, destroy after)', done => {
+				loader._filePromiseWrapper.promise.then( () => {
+					loader.file.then( fileInstance => {
+						tryExpect( done, () => {
+							expect( fileInstance ).to.be.null;
+						} );
+					} );
 
-				expect( loader.file ).to.be.undefined;
+					loader._destroy();
+				} );
 			} );
 		} );
 
@@ -460,10 +482,7 @@ describe( 'FileRepository', () => {
 
 				resolveFile( createNativeFileMock() );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					nativeReaderMock.mockSuccess( 'result data' );
-				}, 0 );
+				loader.file.then( () => nativeReaderMock.mockSuccess( 'result data' ) );
 			} );
 
 			it( 'should reject promise when file promise is rejected', done => {
@@ -504,10 +523,7 @@ describe( 'FileRepository', () => {
 					} );
 				} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					loader.abort();
-				}, 0 );
+				loader.file.then( () => loader.abort() );
 			} );
 
 			it( 'should reject promise on reading error (after file promise is resolved)', done => {
@@ -518,10 +534,7 @@ describe( 'FileRepository', () => {
 					} );
 				} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					nativeReaderMock.mockError( 'reading error' );
-				}, 0 );
+				loader.file.then( () => nativeReaderMock.mockError( 'reading error' ) );
 			} );
 
 			it( 'should resolve promise on reading complete (after file promise is resolved)', done => {
@@ -533,10 +546,7 @@ describe( 'FileRepository', () => {
 						} );
 					} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					nativeReaderMock.mockSuccess( 'result data' );
-				}, 0 );
+				loader.file.then( () => nativeReaderMock.mockSuccess( 'result data' ) );
 			} );
 		} );
 
@@ -578,10 +588,7 @@ describe( 'FileRepository', () => {
 
 				resolveFile( createNativeFileMock() );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					adapterMock.mockSuccess( 'result data' );
-				}, 0 );
+				loader.file.then( () => adapterMock.mockSuccess( 'result data' ) );
 			} );
 
 			it( 'should reject promise when file promise is rejected', done => {
@@ -622,10 +629,7 @@ describe( 'FileRepository', () => {
 					} );
 				} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					loader.abort();
-				}, 0 );
+				loader.file.then( () => loader.abort() );
 			} );
 
 			it( 'should reject promise on reading error (after file promise is resolved)', done => {
@@ -636,10 +640,7 @@ describe( 'FileRepository', () => {
 					} );
 				} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					adapterMock.mockError( 'uploading error' );
-				}, 0 );
+				loader.file.then( () => adapterMock.mockError( 'uploading error' ) );
 			} );
 
 			it( 'should resolve promise on reading complete (after file promise is resolved)', done => {
@@ -651,10 +652,7 @@ describe( 'FileRepository', () => {
 						} );
 					} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					adapterMock.mockSuccess( 'result data' );
-				}, 0 );
+				loader.file.then( () => adapterMock.mockSuccess( 'result data' ) );
 			} );
 
 			it( 'should monitor upload progress', done => {
@@ -666,8 +664,7 @@ describe( 'FileRepository', () => {
 						} );
 					} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
+				loader.file.then( () => {
 					expect( loader.uploaded ).to.equal( 0 );
 					expect( loader.uploadTotal ).to.be.null;
 
@@ -680,7 +677,7 @@ describe( 'FileRepository', () => {
 					expect( loader.uploadTotal ).to.equal( 10 );
 
 					adapterMock.mockSuccess( 'result data' );
-				}, 0 );
+				} );
 			} );
 		} );
 
@@ -716,10 +713,7 @@ describe( 'FileRepository', () => {
 						} );
 					} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					nativeReaderMock.mockSuccess( 'result data' );
-				}, 0 );
+				loader.file.then( () => nativeReaderMock.mockSuccess( 'result data' ) );
 			} );
 
 			it( 'should abort correctly after successful upload', done => {
@@ -737,10 +731,7 @@ describe( 'FileRepository', () => {
 						} );
 					} );
 
-				// Defer so file promise can be resolved.
-				setTimeout( () => {
-					adapterMock.mockSuccess( 'result data' );
-				}, 0 );
+				loader.file.then( () => adapterMock.mockSuccess( 'result data' ) );
 			} );
 		} );
 	} );