From 1aac49db1ca9ea914f9fc44f00e295474faf3ed2 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 3 Oct 2019 15:50:56 +0200 Subject: [PATCH 1/7] Added support for creating a Collection with initial items and supplying multiple items to Collection#add(). --- src/collection.js | 179 +++++++++++++++++++++++++++++++++----------- tests/collection.js | 101 ++++++++++++++++++++++--- 2 files changed, 228 insertions(+), 52 deletions(-) diff --git a/src/collection.js b/src/collection.js index bc22a90..8d6a8f7 100644 --- a/src/collection.js +++ b/src/collection.js @@ -28,10 +28,45 @@ export default class Collection { /** * Creates a new Collection instance. * - * @param {Object} [options={}] The options object. - * @param {String} [options.idProperty='id'] The name of the property which is considered to identify an item. + * You can provide an array of initial items the collection will be created with: + * + * const collection = new Collection( [ { id: 'John' }, { id: 'Mike' } ] ); + * + * console.log( collection.get( 0 ) ); // -> { id: 'John' } + * console.log( collection.get( 1 ) ); // -> { id: 'Mike' } + * console.log( collection.get( 'Mike' ) ); // -> { id: 'Mike' } + * + * Or you can first create a collection and then add new items using the {@link #add} method: + * + * const collection = new Collection(); + * + * collection.add( { id: 'John' } ); + * console.log( collection.get( 0 ) ); // -> { id: 'John' } + * + * Whatever option you choose, you can always pass a configuration object as the last argument + * of the constructor: + * + * const emptyCollection = new Collection( { idProperty: 'name' } ); + * emptyCollection.add( { name: 'John' } ); + * console.log( collection.get( 'John' ) ); // -> { name: 'John' } + * + * const nonEmptyCollection = new Collection( [ { name: 'John' } ], { idProperty: 'name' } ); + * nonEmptyCollection.add( { name: 'George' } ); + * console.log( collection.get( 'George' ) ); // -> { name: 'George' } + * + * @param {Array.|Object} initialItemsOrOptions The initial items of the collection or + * the options object. + * @param {Object} [options={}] The options object, when the first argument is an array of initial items. + * @param {String} [options.idProperty='id'] The name of the property which is used to identify an item. + * Items that do not have such a property will be assigned one when added to the collection. */ - constructor( options = {} ) { + constructor( initialItemsOrOptions = {}, options = {} ) { + const hasInitialItems = initialItemsOrOptions instanceof Array; + + if ( !hasInitialItems ) { + options = initialItemsOrOptions; + } + /** * The internal list of items in the collection. * @@ -88,6 +123,14 @@ export default class Collection { */ this._skippedIndexesFromExternal = []; + // Set the initial content of the collection (if provided in the constructor). + if ( hasInitialItems ) { + for ( const item of initialItemsOrOptions ) { + this._items.push( item ); + this._itemMap.set( this._getItemIdBeforeAdding( item ), item ); + } + } + /** * A collection instance this collection is bound to as a result * of calling {@link #bindTo} method. @@ -125,61 +168,72 @@ export default class Collection { } /** - * Adds an item into the collection. + * Adds an item or multiple items into the collection. + * + * const collection = new Collection(); + * + * collection.add( { id: 'John' } ); + * collection.add( { id: 'Anna' }, { id: 'George' } ); + * + * console.log( collection.length ); // -> 3 + * console.log( collection.get( 2 ) ); // -> { name: 'George' } * - * If the item does not have an id, then it will be automatically generated and set on the item. + * You can specify the index of the item (or items) when adding to the collection: + * + * const collection = new Collection(); + * + * collection.add( { id: 'John' } ); + * collection.add( { id: 'Bob' }, 0 ); + * collection.add( { id: 'Anna' }, { id: 'George' }, 1 ); + * + * console.log( collection.get( 0 ) ); // -> { name: 'Bob' } + * console.log( collection.get( 1 ) ); // -> { name: 'Anna' } + * console.log( collection.get( 2 ) ); // -> { name: 'George' } + * console.log( collection.get( 3 ) ); // -> { name: 'John' } + * + * **Note**: If an item does not have an id, it will be automatically given one (see {@link #constructor}). * * @chainable - * @param {Object} item - * @param {Number} [index] The position of the item in the collection. The item - * is pushed to the collection when `index` not specified. + * @param {...(Object)} items Item or items to be added to the collection. + * @param {Number} [index] The position of the item (or items) when added to the collection. If not specified, + * the item (or items) is pushed to the collection. * @fires add */ - add( item, index ) { - let itemId; - const idProperty = this._idProperty; - - if ( ( idProperty in item ) ) { - itemId = item[ idProperty ]; + add( ...args ) { + let addIndex = args[ args.length - 1 ]; + const items = args; - if ( typeof itemId != 'string' ) { + // E.g. add( { ... }, { ... }, ..., 3 ) + if ( typeof addIndex === 'number' ) { + if ( addIndex > this._items.length || addIndex < 0 ) { /** - * This item's id should be a string. + * The index number has invalid value. * - * @error collection-add-invalid-id + * @error collection-add-item-bad-index + * @param {Number} index The index at which the item is to be added. + * @param {module:utils/collection~Collection} collection The collection the item is added to. */ - throw new CKEditorError( 'collection-add-invalid-id', this ); + throw new CKEditorError( 'collection-add-item-invalid-index', { + index: addIndex, + collection: this + } ); } - if ( this.get( itemId ) ) { - /** - * This item already exists in the collection. - * - * @error collection-add-item-already-exists - */ - throw new CKEditorError( 'collection-add-item-already-exists', this ); - } - } else { - item[ idProperty ] = itemId = uid(); + // The last argument was an index, remove it from the items. + items.pop(); } - // TODO: Use ES6 default function argument. - if ( index === undefined ) { - index = this._items.length; - } else if ( index > this._items.length || index < 0 ) { - /** - * The index number has invalid value. - * - * @error collection-add-item-bad-index - */ - throw new CKEditorError( 'collection-add-item-invalid-index', this ); + // E.g. add( { ... }, { ... } ) + else { + addIndex = this._items.length; } - this._items.splice( index, 0, item ); - - this._itemMap.set( itemId, item ); + this._items.splice( addIndex, 0, ...items ); - this.fire( 'add', item, index ); + items.forEach( ( item, itemIndex ) => { + this._itemMap.set( this._getItemIdBeforeAdding( item ), item ); + this.fire( 'add', item, addIndex + itemIndex ); + } ); return this; } @@ -604,6 +658,47 @@ export default class Collection { } ); } + _getItemIdBeforeAdding( item ) { + const idProperty = this._idProperty; + let itemId; + + if ( ( idProperty in item ) ) { + itemId = item[ idProperty ]; + + if ( typeof itemId != 'string' ) { + /** + * This item's id should be a string. + * + * @error collection-add-invalid-id + * @param {Object} item The item being added to the collection. + * @param {module:utils/collection~Collection} collection The collection the item is added to. + */ + throw new CKEditorError( 'collection-add-invalid-id', { + item, + collection: this, + } ); + } + + if ( this.get( itemId ) ) { + /** + * This item already exists in the collection. + * + * @error collection-add-item-already-exists + * @param {Object} item The item being added to the collection. + * @param {module:utils/collection~Collection} collection The collection the item is added to. + */ + throw new CKEditorError( 'collection-add-item-already-exists', { + item, + collection: this + } ); + } + } else { + item[ idProperty ] = itemId = uid(); + } + + return itemId; + } + /** * Iterable interface. * diff --git a/tests/collection.js b/tests/collection.js index 0dc77c6..16ac87b 100644 --- a/tests/collection.js +++ b/tests/collection.js @@ -15,7 +15,7 @@ function getItem( id, idProperty ) { }; } -describe( 'Collection', () => { +describe.only( 'Collection', () => { let collection; testUtils.createSinonSandbox(); @@ -25,18 +25,44 @@ describe( 'Collection', () => { } ); describe( 'constructor()', () => { - it( 'allows to change the id property used by the collection', () => { - const item1 = { id: 'foo', name: 'xx' }; - const item2 = { id: 'foo', name: 'yy' }; - const collection = new Collection( { idProperty: 'name' } ); - - collection.add( item1 ); - collection.add( item2 ); + it( 'allows setting initial collection items', () => { + const item1 = getItem( 'foo' ); + const item2 = getItem( 'bar' ); + const collection = new Collection( [ item1, item2 ] ); expect( collection ).to.have.length( 2 ); - expect( collection.get( 'xx' ) ).to.equal( item1 ); - expect( collection.remove( 'yy' ) ).to.equal( item2 ); + expect( collection.get( 0 ) ).to.equal( item1 ); + expect( collection.get( 1 ) ).to.equal( item2 ); + expect( collection.get( 'foo' ) ).to.equal( item1 ); + expect( collection.get( 'bar' ) ).to.equal( item2 ); + } ); + + describe( 'options', () => { + it( 'allow to change the id property used by the collection', () => { + const item1 = { id: 'foo', name: 'xx' }; + const item2 = { id: 'foo', name: 'yy' }; + const collection = new Collection( { idProperty: 'name' } ); + + collection.add( item1 ); + collection.add( item2 ); + + expect( collection ).to.have.length( 2 ); + + expect( collection.get( 'xx' ) ).to.equal( item1 ); + expect( collection.remove( 'yy' ) ).to.equal( item2 ); + } ); + + it( 'allow to change the id property used by the collection (initial items)', () => { + const item1 = { id: 'foo', name: 'xx' }; + const item2 = { id: 'foo', name: 'yy' }; + const collection = new Collection( [ item1, item2 ], { idProperty: 'name' } ); + + expect( collection ).to.have.length( 2 ); + + expect( collection.get( 'xx' ) ).to.equal( item1 ); + expect( collection.remove( 'yy' ) ).to.equal( item2 ); + } ); } ); } ); @@ -298,6 +324,61 @@ describe( 'Collection', () => { sinon.assert.calledWithExactly( spy, sinon.match.has( 'source', collection ), item, 1 ); } ); + + describe( 'with multiple items', () => { + it( 'adds multiple items to the collection', () => { + const collection = new Collection(); + const item1 = getItem( 'foo' ); + const item2 = getItem( 'bar' ); + + collection.add( item1, item2 ); + + expect( collection ).to.have.length( 2 ); + expect( collection.get( 0 ) ).to.equal( item1 ); + expect( collection.get( 1 ) ).to.equal( item2 ); + expect( collection.get( 'foo' ) ).to.equal( item1 ); + expect( collection.get( 'bar' ) ).to.equal( item2 ); + } ); + + it( 'adds multiple items to the collection (pushing)', () => { + const collection = new Collection( [ getItem( 'first' ) ] ); + const item1 = getItem( 'foo' ); + const item2 = getItem( 'bar' ); + + collection.add( item1, item2 ); + + expect( collection ).to.have.length( 3 ); + expect( collection.get( 0 ).id ).to.equal( 'first' ); + expect( collection.get( 1 ) ).to.equal( item1 ); + expect( collection.get( 2 ) ).to.equal( item2 ); + expect( collection.get( 'foo' ) ).to.equal( item1 ); + expect( collection.get( 'bar' ) ).to.equal( item2 ); + } ); + + it( 'adds multiple items to the collection at specific position', () => { + const first = getItem( 'first' ); + const last = getItem( 'last' ); + + const collection = new Collection( [ + first, + last + ] ); + + const item1 = getItem( 'foo' ); + const item2 = getItem( 'bar' ); + + collection.add( item1, item2, 1 ); + + expect( collection ).to.have.length( 4 ); + expect( collection.get( 0 ) ).to.equal( first ); + expect( collection.get( 1 ) ).to.equal( item1 ); + expect( collection.get( 2 ) ).to.equal( item2 ); + expect( collection.get( 3 ) ).to.equal( last ); + + expect( collection.get( 'foo' ) ).to.equal( item1 ); + expect( collection.get( 'bar' ) ).to.equal( item2 ); + } ); + } ); } ); describe( 'get()', () => { From 61f59f46a7c9fe14a796c7058bdce25c872abbb1 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 3 Oct 2019 15:53:53 +0200 Subject: [PATCH 2/7] Tests: Added a new test to check if #add is fired for all new items added to the Collection. --- tests/collection.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/collection.js b/tests/collection.js index 16ac87b..e6f326f 100644 --- a/tests/collection.js +++ b/tests/collection.js @@ -378,6 +378,20 @@ describe.only( 'Collection', () => { expect( collection.get( 'foo' ) ).to.equal( item1 ); expect( collection.get( 'bar' ) ).to.equal( item2 ); } ); + + it( 'fires #add for all new items', () => { + const spy = sinon.spy(); + const collection = new Collection( [ getItem( 'first' ) ] ); + const item1 = getItem( 'foo' ); + const item2 = getItem( 'bar' ); + + collection.on( 'add', spy ); + collection.add( item1, item2 ); + + sinon.assert.calledTwice( spy ); + sinon.assert.calledWithExactly( spy.firstCall, sinon.match.has( 'source', collection ), item1, 1 ); + sinon.assert.calledWithExactly( spy.secondCall, sinon.match.has( 'source', collection ), item2, 2 ); + } ); } ); } ); From ef1870cb1104dbb01b4dac30569a803365c9540b Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 3 Oct 2019 15:58:14 +0200 Subject: [PATCH 3/7] Docs: Documented private method in the Collection class. --- src/collection.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/collection.js b/src/collection.js index 8d6a8f7..10baf90 100644 --- a/src/collection.js +++ b/src/collection.js @@ -658,6 +658,14 @@ export default class Collection { } ); } + /** + * For a given item to be added to the collection, it validates item's "id" property or generates + * a new one if missing. If the validation was successful or a new id was generated, + * the id is returned by the method. Otherwise, an error is thrown. + * + * @private + * @param {Object} item An item to be added to the collection. + */ _getItemIdBeforeAdding( item ) { const idProperty = this._idProperty; let itemId; From 821655037b70b57bd927a4a0d8973c98e1eae5c2 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 3 Oct 2019 15:58:49 +0200 Subject: [PATCH 4/7] Tests: Re-enabled all package tests. --- tests/collection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/collection.js b/tests/collection.js index e6f326f..69b7753 100644 --- a/tests/collection.js +++ b/tests/collection.js @@ -15,7 +15,7 @@ function getItem( id, idProperty ) { }; } -describe.only( 'Collection', () => { +describe( 'Collection', () => { let collection; testUtils.createSinonSandbox(); From 798a434b48bd6823d3fb2157172abd6abdb425e7 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 16 Oct 2019 16:21:47 +0200 Subject: [PATCH 5/7] Code refactoring and test improvements. --- src/collection.js | 3 +-- tests/collection.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/collection.js b/src/collection.js index 10baf90..f9a3ab9 100644 --- a/src/collection.js +++ b/src/collection.js @@ -73,7 +73,7 @@ export default class Collection { * @private * @member {Object[]} */ - this._items = []; + this._items = hasInitialItems ? [ ...initialItemsOrOptions ] : []; /** * The internal map of items in the collection. @@ -126,7 +126,6 @@ export default class Collection { // Set the initial content of the collection (if provided in the constructor). if ( hasInitialItems ) { for ( const item of initialItemsOrOptions ) { - this._items.push( item ); this._itemMap.set( this._getItemIdBeforeAdding( item ), item ); } } diff --git a/tests/collection.js b/tests/collection.js index 69b7753..b2af6c7 100644 --- a/tests/collection.js +++ b/tests/collection.js @@ -38,6 +38,18 @@ describe( 'Collection', () => { expect( collection.get( 'bar' ) ).to.equal( item2 ); } ); + it( 'does not use the initial items array internally', () => { + const item1 = getItem( 'foo' ); + const item2 = getItem( 'bar' ); + const initialItems = [ item1, item2 ]; + const collection = new Collection( initialItems ); + + initialItems.pop(); + + expect( collection.get( 0 ) ).to.equal( item1 ); + expect( collection.get( 1 ) ).to.equal( item2 ); + } ); + describe( 'options', () => { it( 'allow to change the id property used by the collection', () => { const item1 = { id: 'foo', name: 'xx' }; From 1b7f9534d0c6c98e83fe4596e2507003bff67522 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 16 Oct 2019 16:25:15 +0200 Subject: [PATCH 6/7] Code refactoring. --- src/collection.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/collection.js b/src/collection.js index f9a3ab9..9306dcc 100644 --- a/src/collection.js +++ b/src/collection.js @@ -61,9 +61,11 @@ export default class Collection { * Items that do not have such a property will be assigned one when added to the collection. */ constructor( initialItemsOrOptions = {}, options = {} ) { - const hasInitialItems = initialItemsOrOptions instanceof Array; + let initialItems = []; - if ( !hasInitialItems ) { + if ( initialItemsOrOptions instanceof Array ) { + initialItems = initialItemsOrOptions; + } else { options = initialItemsOrOptions; } @@ -73,7 +75,7 @@ export default class Collection { * @private * @member {Object[]} */ - this._items = hasInitialItems ? [ ...initialItemsOrOptions ] : []; + this._items = [ ...initialItems ]; /** * The internal map of items in the collection. @@ -124,10 +126,8 @@ export default class Collection { this._skippedIndexesFromExternal = []; // Set the initial content of the collection (if provided in the constructor). - if ( hasInitialItems ) { - for ( const item of initialItemsOrOptions ) { - this._itemMap.set( this._getItemIdBeforeAdding( item ), item ); - } + for ( const item of initialItems ) { + this._itemMap.set( this._getItemIdBeforeAdding( item ), item ); } /** From 28b90175de8149b366e394f7768ae841f38d1c11 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 16 Oct 2019 17:45:27 +0200 Subject: [PATCH 7/7] Docs: Improved Collection docs. --- src/collection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/collection.js b/src/collection.js index 9306dcc..a95bd5a 100644 --- a/src/collection.js +++ b/src/collection.js @@ -54,7 +54,7 @@ export default class Collection { * nonEmptyCollection.add( { name: 'George' } ); * console.log( collection.get( 'George' ) ); // -> { name: 'George' } * - * @param {Array.|Object} initialItemsOrOptions The initial items of the collection or + * @param {Array.|Object} [initialItemsOrOptions] The initial items of the collection or * the options object. * @param {Object} [options={}] The options object, when the first argument is an array of initial items. * @param {String} [options.idProperty='id'] The name of the property which is used to identify an item.