From 615d8b2151a326ba7ef0a29a7d06319b6c54feb7 Mon Sep 17 00:00:00 2001 From: Chris Joel Date: Tue, 8 Sep 2015 14:01:28 -0700 Subject: [PATCH 1/5] Make the tests faile. Guard against random null. --- firebase-collection.html | 2 +- test/firebase-collection.html | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/firebase-collection.html b/firebase-collection.html index 64de6f6..d13041a 100644 --- a/firebase-collection.html +++ b/firebase-collection.html @@ -300,7 +300,7 @@

[[dinosaur.__firebaseKey__]]

return; } - if (pathParts[1] === 'splices') { + if (pathParts[1] === 'splices' && change.value.indexSplices != null) { this._applySplicesToRemoteData(change.value.indexSplices); return; } diff --git a/test/firebase-collection.html b/test/firebase-collection.html index 8aecae4..af5aff4 100644 --- a/test/firebase-collection.html +++ b/test/firebase-collection.html @@ -102,6 +102,11 @@ waitForEvent(firebase, 'firebase-value').then(function() { waitForEvent(firebase, 'firebase-child-added').then(function() { expect(dom.querySelectorAll('div').length).to.be.equal(firebase.data.length); + // NOTE(cdata): This line may seem innocuous but it is + // important for this test. It is possible for an exception to be + // thrown when a value is removed that hasn't been properly + // marked as part of the remote Firebase store: + domBind.shift('data'); done(); }); domBind.unshift('data', { value: 'blah' }); @@ -178,6 +183,21 @@ }); }); + test('works for data bound changes', function(done) { + var intendedValue = firebase.data.length; + var index; + + waitForEvent(firebase, 'firebase-child-added').then(function() { + var item = firebase.data[index]; + + expect(item).to.have.property('value'); + expect(item.value).to.be.equal(intendedValue); + done(); + }); + + index = firebase.push('data', intendedValue) - 1; + }); + test('can be done with `add`', function(done) { var length = firebase.data.length; var newValue = firebase.data[firebase.data.length - 1].value + 1; From 8d58fcce9ca3a8f994e9bc664d7074ee0a4a814d Mon Sep 17 00:00:00 2001 From: Chris Joel Date: Tue, 8 Sep 2015 15:50:36 -0700 Subject: [PATCH 2/5] Fix them tests. --- firebase-collection.html | 7 ++++++- firebase-query-behavior.html | 5 ----- test/firebase-collection.html | 28 ++++++++++++++++++---------- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/firebase-collection.html b/firebase-collection.html index d13041a..be9d263 100644 --- a/firebase-collection.html +++ b/firebase-collection.html @@ -261,7 +261,10 @@

[[dinosaur.__firebaseKey__]]

*/ remove: function(data) { if (data == null || data.__firebaseKey__ == null) { - this._error('Failed to remove unknown value:', data); + // NOTE(cdata): This might be better as an error message in the + // console, but `Polymer.Base._error` throws, which we don't want + // to happen in this case. + this._log('Refusing to remove unknown value:', data); return; } @@ -326,6 +329,8 @@

[[dinosaur.__firebaseKey__]]

this.remove(removed); }, this); + this.splice('data', splice.index, splice.addedCount); + added.forEach(function(added) { this.add(added); }, this); diff --git a/firebase-query-behavior.html b/firebase-query-behavior.html index bfba352..d9f4f67 100644 --- a/firebase-query-behavior.html +++ b/firebase-query-behavior.html @@ -74,9 +74,6 @@ }, _applyRemoteDataChange: function(applyChange) { - if (this._applyingLocalDataChanges) { - return; - } this._receivingRemoteChanges = true; applyChange.call(this); this._receivingRemoteChanges = false; @@ -87,9 +84,7 @@ return; } - this._applyingLocalDataChanges = true; this._applyLocalDataChanges(changes); - this._applyingLocalDataChanges = false; }, _queryChanged: function(query, oldQuery) { diff --git a/test/firebase-collection.html b/test/firebase-collection.html index af5aff4..955a556 100644 --- a/test/firebase-collection.html +++ b/test/firebase-collection.html @@ -101,13 +101,17 @@ test('added values reflect 1-to-1 in the DOM', function(done) { waitForEvent(firebase, 'firebase-value').then(function() { waitForEvent(firebase, 'firebase-child-added').then(function() { - expect(dom.querySelectorAll('div').length).to.be.equal(firebase.data.length); - // NOTE(cdata): This line may seem innocuous but it is - // important for this test. It is possible for an exception to be - // thrown when a value is removed that hasn't been properly - // marked as part of the remote Firebase store: - domBind.shift('data'); - done(); + try { + expect(dom.querySelectorAll('div').length).to.be.equal(firebase.data.length); + // NOTE(cdata): This line may seem innocuous but it is + // important for this test. It is possible for an exception to be + // thrown when a value is removed that hasn't been properly + // marked as part of the remote Firebase store: + domBind.shift('data'); + done(); + } catch (e) { + done(e); + } }); domBind.unshift('data', { value: 'blah' }); }); @@ -190,9 +194,13 @@ waitForEvent(firebase, 'firebase-child-added').then(function() { var item = firebase.data[index]; - expect(item).to.have.property('value'); - expect(item.value).to.be.equal(intendedValue); - done(); + try { + expect(item).to.have.property('value'); + expect(item.value).to.be.equal(intendedValue); + done(); + } catch (e) { + done(e); + } }); index = firebase.push('data', intendedValue) - 1; From 718174783a3b2dc1fb92264d12150fa8184ff7d3 Mon Sep 17 00:00:00 2001 From: Chris Joel Date: Tue, 6 Oct 2015 11:34:58 -0700 Subject: [PATCH 3/5] Clean up the test suite for firebase-collection --- test/firebase-collection.html | 365 ++++++++++++++-------------------- test/test-helpers.html | 68 +++++++ 2 files changed, 216 insertions(+), 217 deletions(-) diff --git a/test/firebase-collection.html b/test/firebase-collection.html index 955a556..95f675c 100644 --- a/test/firebase-collection.html +++ b/test/firebase-collection.html @@ -17,7 +17,6 @@ - @@ -25,42 +24,7 @@ - - - - - - - - - - @@ -75,7 +39,6 @@
[[item.value]]
@@ -85,245 +48,213 @@ diff --git a/test/test-helpers.html b/test/test-helpers.html index bfd28b0..735137b 100644 --- a/test/test-helpers.html +++ b/test/test-helpers.html @@ -1,14 +1,82 @@ + From 2825ebd64a3656b453500de48a924c455ed58f74 Mon Sep 17 00:00:00 2001 From: Chris Joel Date: Sun, 11 Oct 2015 11:55:09 -0700 Subject: [PATCH 4/5] Re-arrange asynchrony to avoid re-entrancy. --- firebase-collection.html | 125 +++++++++++++++++++++++++++------- firebase-document.html | 8 ++- firebase-query-behavior.html | 31 ++++++--- test/firebase-collection.html | 51 +++++++++----- 4 files changed, 161 insertions(+), 54 deletions(-) diff --git a/firebase-collection.html b/firebase-collection.html index be9d263..612fc83 100644 --- a/firebase-collection.html +++ b/firebase-collection.html @@ -233,6 +233,11 @@

[[dinosaur.__firebaseKey__]]

'firebase-child-moved': '_onFirebaseChildMoved', }, + created: function() { + this._pendingSplices = []; + this._lastLocallyAddedIndex = null; + }, + /** * Add an item to the document referenced at `location`. A key associated * with the item will be created by Firebase, and can be accessed via the @@ -264,7 +269,7 @@

[[dinosaur.__firebaseKey__]]

// NOTE(cdata): This might be better as an error message in the // console, but `Polymer.Base._error` throws, which we don't want // to happen in this case. - this._log('Refusing to remove unknown value:', data); + this._warn('Refusing to remove unknown value:', data); return; } @@ -290,51 +295,82 @@

[[dinosaur.__firebaseKey__]]

* located at `location`. */ removeByKey: function(key) { + if (!this.query) { + this._error('Cannot remove items before query has been initialized.'); + return; + } + this.query.ref().child(key).remove(); }, - _applyLocalDataChanges: function(change) { + _localDataChanged: function(change) { var pathParts = change.path.split('.'); - var firebaseKey; - var key; - var value; + // We don't care about self-changes, and we don't respond directly to + // length changes: if (pathParts.length < 2 || pathParts[1] === 'length') { return; } + // Handle splices via the adoption process. `indexSplices` is known to + // sometimes be null, so guard against that. if (pathParts[1] === 'splices' && change.value.indexSplices != null) { - this._applySplicesToRemoteData(change.value.indexSplices); + this._adoptSplices(change.value.indexSplices); return; } - key = pathParts[1]; - value = Polymer.Collection.get(change.base).getItem(key); + // Otherwise, the change is happening to a sub-path of the array. + this._applySubPathChange(change); + }, + + _applySubPathChange: function(change) { + var key = change.path.split('.')[1]; + var value = Polymer.Collection.get(change.base).getItem(key); + var firebaseKey = value.__firebaseKey__; - // Temporarily remove the client-only `__firebaseKey__` property: - firebaseKey = value.__firebaseKey__; + // We don't want to accidentally reflect `__firebaseKey__` in the + // remote data, so we remove it temporarily. `null` values will be + // discarded by Firebase, so `delete` is not necessary: value.__firebaseKey__ = null; - this.query.ref().child(firebaseKey).set(value); - value.__firebaseKey__ = firebaseKey; }, - _applySplicesToRemoteData: function(splices) { - this._log('splices', splices); - splices.forEach(function(splice) { - var added = splice.object.slice(splice.index, splice.index + splice.addedCount); + _adoptSplices: function(splices) { + this._pendingSplices = this._pendingSplices.concat(splices); - splice.removed.forEach(function(removed) { - this.remove(removed); + // We can afford apply removals synchronously, so we do that first + // and save the `added` operations for the `debounce` below. + this._applyLocalDataChange(function() { + splices.forEach(function(splice) { + splice.removed.forEach(function(removed) { + this.remove(removed); + }, this); }, this); + }); - this.splice('data', splice.index, splice.addedCount); + // We async until the next turn. The reason we want to do this is + // that splicing within a splice handler will break the data binding + // system in some places. This is referred to as the "re-entrancy" + // problem. See polymer/polymer#2491. + this.debounce('_adoptSplices', function() { + this._applyLocalDataChange(function() { + var splices = this._pendingSplices; - added.forEach(function(added) { - this.add(added); - }, this); - }, this); + this._pendingSplices = []; + + splices.forEach(function(splice) { + var added = splice.object.slice(splice.index, splice.index + splice.addedCount); + + added.forEach(function(added, index) { + this._lastLocallyAddedIndex = splice.index + index; + this.add(added); + }, this); + }, this); + + this._lastLocallyAddedIndex = null; + }); + }); }, _computeQuery: function(location, limitToFirst, limitToLast, orderByMethodName, startAt, endAt, equalTo) { @@ -344,6 +380,8 @@

[[dinosaur.__firebaseKey__]]

return; } + this._log('Recomputing query.', arguments); + query = new Firebase(location); if (orderByMethodName) { @@ -465,17 +503,56 @@

[[dinosaur.__firebaseKey__]]

_onFirebaseChildAdded: function(event) { this._applyRemoteDataChange(function() { var value = this._valueFromSnapshot(event.detail.childSnapshot); + var key = value.__firebaseKey__; var previousValueKey = event.detail.previousChildName; var index = previousValueKey != null ? this.data.indexOf(this._valueMap[previousValueKey]) + 1 : 0; + var lastLocallyAddedValue; this._valueMap[value.__firebaseKey__] = value; - this.splice('data', index, 0, value); + // NOTE(cdata): The rationale for this conditional dance around the + // last locally added index (since you will inevitably be wondering + // why we do it): + // There may be a "locally" added value which was spliced in. If + // there is, it may be in the "wrong" place in the array. This is + // due to the fact that Firebase may be applying a sort to the + // data, so we won't know the correct index for the locally added + // value until the `child_added` event is fired. + // Once we get the `child_added` event, we can check to see if the + // locally added value is in the right place. If it is, we just + // `set` it to the Firebase-provided value. If it is not, then + // we grab the original value, splice in the Firebase-provided + // value in the right place, and then (importantly: at the end) + // find the locally-added value again (since its index may have + // changed by splicing-in Firebase's value) and splice it out of the + // array. + if (this._lastLocallyAddedIndex === index) { + this.set(['data', index], value); + } else { + if (this._lastLocallyAddedIndex != null) { + lastLocallyAddedValue = this.data[this._lastLocallyAddedIndex]; + } + + this.splice('data', index, 0, value); + + if (this._lastLocallyAddedIndex != null) { + this.splice('data', this.data.indexOf(lastLocallyAddedValue), 1); + } + } }); }, _onFirebaseChildRemoved: function(event) { + if (this._receivingLocalChanges) { + this._valueMap[event.detail.oldChildSnapshot.key()] = null; + // NOTE(cdata): If we are receiving local changes, that means that + // the splices have already been performed and items are already + // removed from the local data representation. No need to remove + // them again. + return; + } + this._applyRemoteDataChange(function() { var key = event.detail.oldChildSnapshot.key(); var value = this._valueMap[key]; diff --git a/firebase-document.html b/firebase-document.html index de4739f..ff2010d 100644 --- a/firebase-document.html +++ b/firebase-document.html @@ -64,7 +64,7 @@ 'firebase-value': '_onFirebaseValue' }, - _applyLocalDataChanges: function(change) { + _localDataChanged: function(change) { var pathFragments = change.path.split('.'); if (pathFragments.length === 1) { @@ -98,8 +98,10 @@ }, _setRemoteDocumentChild: function(key, value) { - this._log('Setting child "' + key + '" to', value); - this.query.child(key).set(value); + this.debounce('set-' + key, function() { + this._log('Setting child "' + key + '" to', value); + this.query.child(key).set(value); + }); }, _removeRemoteDocumentChild: function(key) { diff --git a/firebase-query-behavior.html b/firebase-query-behavior.html index d9f4f67..7c67243 100644 --- a/firebase-query-behavior.html +++ b/firebase-query-behavior.html @@ -40,11 +40,6 @@ type: Boolean, value: false, reflectToAttribute: true - }, - - _receivingRemoteChanges: { - type: Boolean, - value: false } }, @@ -52,6 +47,11 @@ '_dataChanged(data.*)' ], + created: function() { + this._receivingLocalChanges = false; + this._receivingRemoteChanges = false; + }, + get dataAsObject() { if (Array.isArray(this.data)) { return this.data.reduce(function(object, value, index) { @@ -69,10 +69,16 @@ this.location = ''; }, - _applyLocalDataChanges: function(changes) { + _localDataChanged: function(changes) { // Virtual.. }, + _applyLocalDataChange: function(applyChange) { + this._receivingLocalChanges = true; + applyChange.call(this); + this._receivingLocalChanges = false; + }, + _applyRemoteDataChange: function(applyChange) { this._receivingRemoteChanges = true; applyChange.call(this); @@ -80,11 +86,12 @@ }, _dataChanged: function(changes) { - if (this._receivingRemoteChanges) { + if (this._receivingRemoteChanges || + this._receivingLocalChanges) { return; } - this._applyLocalDataChanges(changes); + this._localDataChanged(changes); }, _queryChanged: function(query, oldQuery) { @@ -176,9 +183,15 @@ } }, + _warn: function() { + if (this.log) { + Polymer.Base._warn(this._logf.apply(this, arguments)); + } + }, + _error: function() { if (this.log) { - console.error.apply(console, arguments); + Polymer.Base._error(this._logf.apply(this, arguments)); } } }; diff --git a/test/firebase-collection.html b/test/firebase-collection.html index 95f675c..3866864 100644 --- a/test/firebase-collection.html +++ b/test/firebase-collection.html @@ -116,7 +116,9 @@ test('can be done with `remove`', function() { var objectToBeRemoved = firebase.data[0]; firebase.remove(objectToBeRemoved); + expect(firebase.data.length).to.be.equal(numberOfItems - 1); + expect(firebase.data.indexOf(objectToBeRemoved)).to.be.equal(-1); }); }); @@ -125,31 +127,39 @@ firebase = fixtureFirebase('TrivialCollection'); }); - test('works for data-bound changes', function() { + test('works for data-bound changes', function(done) { var intendedValue = randomInt(); var index = firebase.push('data', intendedValue) - 1; - expect(firebase.data[index]).to.have.property('value'); - expect(firebase.data[index].value).to.be.equal(intendedValue); + // NOTE(cdata): See polymer/polymer#2491. + Polymer.Base.async(function() { + expect(firebase.data[index]).to.have.property('value'); + expect(firebase.data[index].value).to.be.equal(intendedValue); + done(); + }, 1); }); - test('can be done with `add`', function() { + test('can be done with `add`', function(done) { var object = randomObject(); var length = firebase.data.length; var foundObject; firebase.add(object); - expect(firebase.data.length).to.be.equal(length + 1); + // NOTE(cdata): See polymer/polymer#2491. + Polymer.Base.async(function() { + expect(firebase.data.length).to.be.equal(length + 1); - firebase.data.forEach(function(datum) { - if (datum.value === object.value) { - foundObject = datum; - } - }); + firebase.data.forEach(function(datum) { + if (datum.value === object.value) { + foundObject = datum; + } + }); - expect(foundObject).to.be.okay; - expect(foundObject.value).to.be.equal(object.value); + expect(foundObject).to.be.okay; + expect(foundObject.value).to.be.equal(object.value); + done(); + }, 1); }); }); @@ -216,16 +226,21 @@ numberOfItems = 3; }); - test('splices reflect in Firebase data', function() { + test('splices reflect in Firebase data', function(done) { domBind.splice('data', 0, 1, randomObject()); domBind.shift('data'); - domBind.push('data', arrayOfObjects(2)); + domBind.push.apply(domBind, ['data'].concat(arrayOfObjects(2))); + + // NOTE(cdata): See polymer/polymer#2491. + Polymer.Base.async(function() { + expect(firebase.data.length).to.be.equal(domBind.data.length); - expect(firebase.data.length).to.be.equal(domBind.data.length); + firebase.data.forEach(function(datum, index) { + expect(domBind.data[index].value).to.be.equal(datum.value); + }); - firebase.data.forEach(function(datum, index) { - expect(domBind.data[index].value).to.be.equal(datum.value); - }); + done(); + }, 1); }); test('splices reflect in the DOM', function(done) { From b4dd34c667c38c3fe4a963fa1feee26f96e47928 Mon Sep 17 00:00:00 2001 From: Chris Joel Date: Fri, 16 Oct 2015 16:08:58 -0700 Subject: [PATCH 5/5] Add Travis config. --- .travis.yml | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..66864db --- /dev/null +++ b/.travis.yml @@ -0,0 +1,28 @@ +language: node_js +sudo: false +matrix: + include: + - node_js: stable + script: xvfb-run -a wct --simpleOutput -l firefox -l chrome + addons: + firefox: latest + apt: + sources: + - google-chrome + packages: + - google-chrome-stable + - node_js: node + script: + - | + if [ "${TRAVIS_PULL_REQUEST}" = "false" ]; then + wct --simpleOutput -s 'Windows 10/microsoftedge' -s 'Windows 8.1/internet explorer@11' -s 'Windows 7/internet explorer@10' -s 'OS X 10.10/safari@8' -s 'OS X 10.9/safari@7' + fi +before_script: +- npm install bower +- npm install web-component-tester +- export PATH=$PWD/node_modules/.bin:$PATH +- bower install +env: + global: + - secure: dlr2l5btcMEnaJI/VzBDIKhcJTUgnrQ6AN/S+qbPFqsckokV5SCt3k0f0Z22CBhBXJi2qVrLDX05+wZ4FkVkLLdGOf6zR0sSNvXubzvUu8oKaZczo2B8EAs+OjsuvaZWPYTabAOGvyAwuBzfZaYDlXbk6Dlb/51hjzSl2D5/WbY= + - secure: dFreaGtRTAwcFtSZ57ktyiDejTkJ7vI9TVbJZ0Yd9Adp/4mINOOgHDFiVDl5kDCy9xx21vCRel+IrGkOdRllk+OWxO8Ga1OQF4EzchvUsxzngDwi3I0P1+uNuzjn8MyEvE4HYPwDZ0mDYzMLJS9b1GlcpMTvJvE2Sg3ly8h4wYc=