Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firestore #327

Open
wants to merge 21 commits into
base: firestore
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
844dcfd
added exists attribute, reflecting whether we have data at a given pa…
christophe-g Jun 17, 2017
5274aeb
Merge branch 'master' into knowIfExists
christophe-g Sep 7, 2017
91d7793
Update README.md
imarian97 Oct 6, 2017
15f1c8a
Merge pull request #277 from imarian97/patch-1
tjmonsi Oct 7, 2017
f6df335
Fix typo in code
FluorescentHallucinogen Nov 4, 2017
f107307
Merge pull request #305 from FluorescentHallucinogen/patch-2
tjmonsi Nov 5, 2017
f6b0b14
integrate @merlinnot comments
christophe-g Jan 13, 2018
58ed184
integrate @merlinnot comment
christophe-g Jan 13, 2018
c8e301c
integrate @merlinnot comments
christophe-g Jan 13, 2018
ac153f4
Merge branch 'master' into knowIfExists
christophe-g Jan 13, 2018
ab634d9
Merge branch 'master' into knowIfExists
christophe-g Jan 13, 2018
7290611
added tests
christophe-g Jan 13, 2018
a7e445e
Fix indentation in test files.
merlinnot Jan 13, 2018
6ca00b6
Merge pull request #234 from christophe-g/knowIfExists
tjmonsi Feb 21, 2018
ba6a8e8
Merge pull request #312 from merlinnot/fix-issue-311
tjmonsi Feb 21, 2018
7ea2df2
Race condition prevention.
Westbrook Feb 21, 2018
7ff8a37
Prevent collection paths with multiple arguments from triggering mult…
Westbrook Feb 23, 2018
a3f9653
Merge branch 'master' into firestore
Westbrook Feb 24, 2018
63cbcaa
`_firestoreUpdateBinding` every `_firestoreBind`
Westbrook Feb 25, 2018
fd337f1
Lazily unlisten and create DB
Westbrook Feb 25, 2018
24e2d64
Move unlisten back to before the "ready" check
Westbrook Feb 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ workflows.
Example Usage:

```html
<firebase-app
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com/"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
storage-bucket="polymerfire-test.appspot.com"
messaging-sender-id="544817973908"
project-id="polymerfire-test">
</firebase-app>
<firebase-auth id="auth" user="{{user}}" provider="google" on-error="handleError">
</firebase-auth>
```
Expand Down
20 changes: 19 additions & 1 deletion firebase-database-behavior.html
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,23 @@
disabled: {
type: Boolean,
value: false
}
},

/**
* `exists` is set to `true` when the data actually exists for the
* specified path; `false` otherwise.
* When we are unable to determine whether data exists or not
* (e.g. first round trip to the server not yet performed) the value
* is `null`
*/
exists: {
type: Boolean,
notify: true,
value: null,
readOnly: true,
reflectToAttribute: true
},

},

observers: [
Expand Down Expand Up @@ -95,6 +111,8 @@
},

__pathChanged: function(path, oldPath) {
this._setExists(null);

if (!this.disabled && !this.valueIsEmpty(this.data)) {
this.syncToMemory(function() {
this.data = this.zeroValue;
Expand Down
8 changes: 7 additions & 1 deletion firebase-document.html
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,13 @@

__onFirebaseValue: function(snapshot) {
var value = snapshot.val();
var exists = true;

if (value == null) {
value = this.zeroValue;
this.__needSetData = true;
}
exists = false;
}

if (!this.isNew) {
this.async(function() {
Expand All @@ -188,6 +190,10 @@
}
}
});
this._setExists(exists);
if(!exists) {
this.fire('empty-result');
}
});
}
}
Expand Down
10 changes: 4 additions & 6 deletions firebase-firestore-mixin.html
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,10 @@

this._firestoreProps = {};
this._firestoreListeners = {};
this.db = this.constructor.db || firebase.firestore();
}

connectedCallback() {
super.connectedCallback();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why did you decide to move it here? What’s the reasoning behind it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly stole these two lines from #279, but the goal is to not have an issue like this: https://receptive-kitten.glitch.me/ I can look into what this is actually fixing soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually needed to fix it? I can’t really see why would it help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, finally got into this part more. It helps because if you allow super.connectedCallback() to run, then the code at https://github.com/firebase/polymerfire/blob/master/firebase-app.html#L123 will also be allowed to fully run, which means that the app will have actually connected to Firebase. It's possible that you suggestions on moving some of the this.db stuff out to the scope might address this as well. However, were that not so, maybe this is a use case for https://www.polymer-project.org/2.0/docs/api/namespaces/Polymer.RenderStatus#function-Polymer.RenderStatus.afterNextRender ?

if (this[CONNECTED_CALLBACK_TOKEN] !== true) {
this[CONNECTED_CALLBACK_TOKEN] = true;

Expand All @@ -188,8 +188,6 @@
}
}
}

super.connectedCallback();
}

_firestoreBind(name, options) {
Expand All @@ -205,19 +203,17 @@
this._firestoreProps[name] = config;

const args = config.props.concat(config.observes);
this._firestoreUpdateBinding(name, ...args.map(x => this[x]));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought more about the issue at hand here, and it wasn't so much that I didn't want this called in every _firestoreBind call, but that I didn't want it called after the _createMethodObserver had been created because it meant that when _firestoreUnlisten was called as part of the initial _firestoreUpdateBinding call would immediately trigger a second call to _firestoreUpdateBinding because of the zeroing out of the value for the property being bound to at line 251.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at how the observer is created: https://github.com/firebase/polymerfire/pull/327/files#diff-ed26b76bc80e40b84633288e109db714R211

The name is passed in quotes, just to indicate for which property arguments have changed. It does not observe the property itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I didn't follow that through line deep enough.

With the code ordered this way, the initial properties flush of the args is able to be passed before _firestoreUnlisten makes the call to setProperties, which is why it's not called twice at runtime. This is due to the initial call to _flushProperties not occurring till https://github.com/Polymer/polymer/blob/master/lib/mixins/property-effects.html#L1661

Not sure if there is a more idiomatic way of achieving the timing we need here.

if (args.length > 0) {
// Create a method observer that will be called every time
// a templatized or observed property changes
const observer =
`_firestoreUpdateBinding('${name}', ${args.join(',')})`
this._createMethodObserver(observer);
}

this._firestoreUpdateBinding(name, ...args.map(x => this[x]));
}

_firestoreUpdateBinding(name, ...args) {
this._firestoreUnlisten(name);

const config = this._firestoreProps[name];
const isDefined = (x) => x !== undefined;
Expand All @@ -229,6 +225,8 @@
observesArgs.length === config.observes.length;

if (propArgsReady && observesArgsReady) {
this._firestoreUnlisten(name);
this.db = this.db || firebase.firestore();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some moves here to get this less likely to happen on first run.

@merlinnot I see what you're saying now about possibly moving db to the scope. I think the reason it's used like this is to mimic the way firebase-database-behavior.html actually adds db to the properties getter https://github.com/firebase/polymerfire/blob/master/firebase-database-behavior.html#L20 not sure if there's benefits to doing that or not here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. I think we might want to rename this property, but I’ll make an issue for this, let’s not do it as a part of this PR.

Original (but broken) approach using constructor had a good intention of reusing the same firestore instance. What we should do IMO is:

  • create a variable in the scope to cache firestore instance
  • create a getter in the class which would assign a value to this variable as a side-effect if it’s undefined, than return it.
  • use this variable directly within the class methods to avoid calling the getters logic (performance)
  • make sure we initially assign it as late as possible (we might want to extract “assign if undefined”) logic to a helper method to reuse it in the getter and for initial assignment

Does it sound right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense. I'm not quite sure I understand how to achieve what you're talking about in the last two bullets, so I can't say whether I think it's overkill or not. Would this look something like:

if (typeof Polymer === 'undefined') {
  throw new Error('Polymer.FirestoreMixin must be imported after Polymer itself.');
}
{
  let DB;

  Polymer.FirestoreMixin = parent =>
    get db() {
      DB = DB || firebase.firestore();
      return DB;
    }
    initDB() {
      DB ? return : this.db;
    }
    _firestoreUpdateBinding(name, ...args) {
      // ...
      if (propArgsReady && observesArgsReady) {
        this._firestoreUnlisten(name);
        initDB();
        // ...
        let ref = DB[config.type](collPath);
        // ...
      }
      // ...
    }
}

If so, I'm not completely sure how the "assign if undefined" logic in initDB would add any performance benefit over calling this.db directly. Also, correct me if I'm wrong, but doesn't this also assume that every instance of this mixin is then attached to the same Firestore remote? I've not spent much time attaching things to scope like this, so this is great learning for me.

const collPath = stitch(config.literals, propArgs);
const assigner = this._firestoreAssigner(name, config);

Expand Down
17 changes: 12 additions & 5 deletions firebase-query.html
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@
this.syncToMemory(function() {
this.__map = {};
this.set('data', this.zeroValue);
this._setExists(null);
});
}

Expand All @@ -305,13 +306,14 @@
query.off('child_changed', this.__onFirebaseChildChanged, this);
query.off('child_moved', this.__onFirebaseChildMoved, this);
}

this._onOnce = true;
this._query = query

this._setExists(null);
this._onOnce = true;
this._query = query;

// does the on-value first
query.off('value', this.__onFirebaseValue, this)
query.on('value', this.__onFirebaseValue, this.__onError, this)
query.off('value', this.__onFirebaseValue, this);
query.on('value', this.__onFirebaseValue, this.__onError, this);
}
},

Expand All @@ -338,6 +340,7 @@
}.bind(this))

this.set('data', data);
this._setExists(true);
}

const query = this.query
Expand Down Expand Up @@ -371,6 +374,7 @@

this.__map[key] = value;
this.splice('data', previousChildIndex + 1, 0, value);
this._setExists(true);
},

__onFirebaseChildRemoved: function(snapshot) {
Expand All @@ -390,6 +394,9 @@
this.splice('data', this.__indexFromKey(key), 1);
}
});
if (this.data.length === 0) {
this._setExists(false);
}
});
}
},
Expand Down
4 changes: 2 additions & 2 deletions firebase-storage-multiupload.html
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
* file-task
*
* <firebase-storage-upload-task
* task="[[item]]""
* task="[[item]]"
* bytes-transferred="{{bytesTransferred}}"
* total-bytes="{{totalBytes}}"
* state="{{state}}"
Expand All @@ -74,7 +74,7 @@
*
* <template is="dom-repeat" items="[[uploadTasks]]">
* <firebase-storage-upload-task
* task="[[item]]""
* task="[[item]]"
* bytes-transferred="{{item.bytesTransferred}}"
* total-bytes="{{item.totalBytes}}"
* state="{{item.state}}"
Expand Down
8 changes: 4 additions & 4 deletions test/firebase-auth.html
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
<body>

<firebase-app
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
</firebase-app>

<test-fixture id="BasicAuth">
Expand Down
22 changes: 11 additions & 11 deletions test/firebase-common-behavior.html
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,23 @@
<body>

<firebase-app
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
</firebase-app>

<firebase-app
name="alt"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
name="alt"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
</firebase-app>

<firebase-app
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
</firebase-app>

<test-fixture id="BasicCommon">
Expand Down
8 changes: 4 additions & 4 deletions test/firebase-database-behavior.html
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@
<body>

<firebase-app
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
</firebase-app>

<test-fixture id="BasicDatabase">
Expand Down
61 changes: 57 additions & 4 deletions test/firebase-document.html
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
<body>

<firebase-app
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
name="test"
api-key="AIzaSyDTP-eiQezleFsV2WddFBAhF_WEzx_8v_g"
auth-domain="polymerfire-test.firebaseapp.com"
database-url="https://polymerfire-test.firebaseio.com">
</firebase-app>

<test-fixture id="BasicStorage">
Expand Down Expand Up @@ -65,6 +65,59 @@
});
}
});

function pushFirebaseValue(path, value) {
return firebase.app('test').database().ref(path).push(value);
}

function clearFirebaseValue(path) {
return firebase.app('test').database().ref(path).set(null);
}

var makeObject;
var root;

setup(function() {
var objectId = 0;
makeObject = function(value) {
return {
val: value || objectId++
};
};

return pushFirebaseValue('/test', { ignore: 'me' }).then(function(snapshot) {
root = '/test/' + snapshot.key;
});
});

suite('exists attribute', function() {
var query;

setup(function() {
query = fixture('BasicStorage');
query.path = root + '/list';
return query.transactionsComplete;
});

test('exists is null when we change the path', function() {
query.path = '/myNewPath';
expect(query.exists).to.be.equal(null);
});

test('exists is true when we have data false when we remove it.', function() {
var object = makeObject();

return pushFirebaseValue(query.path, object).then(function() {
expect(query.exists).to.be.equal(true);
}).then(function(){
clearFirebaseValue(query.path);
}).then(function() {
expect(query.exists).to.be.equal(false);
});
});

});

});
</script>
</body>
Expand Down
25 changes: 21 additions & 4 deletions test/firebase-query.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
</head>
<body>
<firebase-app
name="test"
api-key="AIzaSyBzKhxNa2k9pA3m9_Ji3POFAKyGGFnyshI"
auth-domain="note-app-firebase-3a483.firebaseapp.com"
database-url="https://note-app-firebase-3a483.firebaseio.com">
name="test"
api-key="AIzaSyBzKhxNa2k9pA3m9_Ji3POFAKyGGFnyshI"
auth-domain="note-app-firebase-3a483.firebaseapp.com"
database-url="https://note-app-firebase-3a483.firebaseio.com">
</firebase-app>

<test-fixture id="BasicQuery">
Expand Down Expand Up @@ -176,6 +176,7 @@
var object = makeObject();

return pushFirebaseValue(query.path, object).then(function() {
expect(query.exists).to.be.equal(true);
expect(query.data.length).to.be.equal(1);
expect(query.data[0]).to.be.ok;
expect(query.data[0].val).to.be.equal(object.val);
Expand Down Expand Up @@ -241,6 +242,22 @@
expect(query.data[0].foo).to.be.eql(undefined);
});
});

test('exists is null when template is stamped', function() {
expect(query.exists).to.be.equal(null);
})

test('exists is true when we have data false when we remove it.', function() {
var object = makeObject();

return pushFirebaseValue(query.path, object).then(function() {
expect(query.exists).to.be.equal(true);
}).then(function(){
clearFirebaseValue(query.path);
}).then(function() {
expect(query.exists).to.be.equal(false);
});
});
});

suite('querying against leaf node collections', function() {
Expand Down