added exists attribute, reflecting whether we have data at a given path by christophe-g · Pull Request #234 · FirebaseExtended/polymerfire

christophe-g · 2017-06-17T18:25:28Z

Replace and close #177.
Follows #33 discussion.

An "empty-result" event is fired by firebase-document or firebase-query to notify that the path does not hold any data.

…th. An "empty-result" event is fired by firebase-document or firebase-query to notify that the path does not hold any data.

christophe-g · 2017-07-20T12:14:13Z

Any new on this one ?
Thanks.

JosefJezek · 2017-07-24T17:44:15Z

@mbleigh FYI

tjmonsi · 2017-09-06T14:40:00Z

@christophe-g can you check it again as it has some conflicts with firebase-query

christophe-g · 2017-09-07T07:49:37Z

@tjmonsi - thanks.
@mbleigh - for your review ; )

tjmonsi

Why not use the exist() method to show that a value on the snapshot really exists given a path?

I could edit this PR and add the ready attribute as well in this PR if you want or you can do it. :)

christophe-g · 2017-10-31T09:05:58Z

@tjmonsi - thanks !
@andrewspy - for info

Why not use the exist() method to show that a value on the snapshot really exists given a path?

I am not sure which exist method you are referring to - if there is one, the reason for not using is certainly that this PR predates an exist implementation.

If you are referring to snapshot.exists, the reason is to better integrate the PR within existing code flow.

For instance, as we already have this (with value == null which is equivalent to snapshot.exists() ):

    __onFirebaseValue: function(snapshot) {
      var value = snapshot.val();
      if (value == null) {
        value = this.zeroValue;
        this.__needSetData = true;
      }

It is pretty natural to set exists to false there, without adding one additional check.

I could edit this PR and add the ready attribute as well in this PR if you want or you can do it. :)

I would rather keep exists instead of ready as it conveys the idea of whether data exists or not. ready just indicates that database round-trip is completed - but I am open to discuss it ; )

tjmonsi · 2017-10-31T10:31:53Z

var value = snapshot.val();
      if (value == null) {
        value = this.zeroValue;
        this.__needSetData = true;
      }

I think we can change this as

this.exists = snapshot.exist();
if (!this.exists) {
  ...
}

I'm still trying to find time for this but I think I can squeeze this in today with ready
If that's alright with you, I'll do the changes.

andrewspy · 2017-10-31T10:52:40Z

@christophe-g @tjmonsi In my opinion, both exists and ready serve a different purpose.

exists should refer to the snapshot.exists() in official firebase Nodejs API to make the API complete, but in Polymerfire land, it is useful for a developer to know when data is ready, with the data guarantee available for data-binding and consumption purpose (assuming the data does exist).

In short, exists is meant to use pragmatically, while ready is meant to use with Polymer + Polymerfire specifically for data-binding and consumption purpose.

christophe-g · 2017-10-31T13:23:52Z

@tjmonsi

If that's alright with you, I'll do the changes.

That is alright - I am pretty stretched!

tjmonsi · 2017-10-31T23:41:07Z

@christophe-g gotyah. Will work on it in a bit

merlinnot · 2018-01-05T22:17:22Z

I see the reasoning behind this, but there are few things I’d like to discuss:

Since the value of exists is based on a value of data, wouldn’t it be better to make it a computed property? Since we cannot use setProperties here to ensure the property changes run as a coherent set I think it would be better to set the value of exists after changes to the data itself are made.
Do we really need this event? Polymer provides us with on-exists-changed, isn’t that enough?
I’d personally prefer to set this value to undefined if it’s unknown. Why did you decide to go with null?

christophe-g · 2018-01-08T08:04:48Z

@merlinnot - thanks for looking at this.

If I remember correctly, I was worried about performance penalties in adding computed properties listening to data changes. As we already indirectly observe data modification in firebase callbacks (__onFirebaseValue for firebase-document and __onChildAdded, __onChildRemoved for firebase-query), I am assuming (without testing) that performance penalties are minimum with current implementation.
I am not using th empty-result event. It was suggested in the related firebase-document defaults to {} before resolving #33 discussion. We can remove it as far as I am concerned.
Polymer 1.0 does not fire computed/observed properties when one of the parameters are undefined. For the 1.0 branch - as exists property notifies and is potentially bound to external components - setting undefined will partially break the purpose of this PR. I am fine with undefined for the 2.0 branch.

merlinnot · 2018-01-08T09:41:09Z

+    /**
+     * the element is notified that the path does not hold any data (fired by firebase-document or firebase query)
+     *
+     * @event empty-result


This event is unnecessary. One can use on-exists-changed which Polymer fires by default.

merlinnot · 2018-01-08T09:45:06Z

+
+        /**
+         * `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`


Could you please split this comment into multiple lines so it's more readable without line wrapping? ❤️

merlinnot · 2018-01-08T09:49:17Z

            value = this.__snapshotToValue(snapshot);

            this.__map[key] = value;
+            this._setExists(true);


This should be moved after an assignment of a value to the data property.

merlinnot · 2018-01-08T09:51:31Z

          var key = snapshot.key;

          if (this.__initialLoadDone) {
+


We don't have to add a new line here.

merlinnot · 2018-01-08T10:04:48Z

              * the callback if the query changes
              */
            query.on('value', this.__onFirebaseValue, this.__onError, this);
+            this._setExists(null);         


This one can be moved immediately after query.offs

merlinnot · 2018-01-08T10:24:54Z

              this.syncToMemory(function() {
                this.splice('data', this.__indexFromKey(key), 1);
              });
+              if (this.data.length === 0) {


Hm... what if data is changed in some property effect? Don't you think we should get the length of data before calling this.splice?

We are just assessing whether we have data or not. Sounds correct to me,

merlinnot · 2018-01-08T10:32:47Z

I see, that's ok.
Let's remove it.
Ok, I'm fine with that. I guess we still have to support v1 guys ;)

Could you please add some basic tests and resolve conflicts? Just a few test 😀 😃

@tjmonsi what about ready? ;)

christophe-g · 2018-01-13T13:03:11Z

@tjmonsi @merlinnot - ready for review

christophe-g · 2018-01-23T08:06:04Z

@merlinnot - any news on this ?

Just a few test 😀 😃

There are even tests !
Thnks

merlinnot · 2018-01-23T12:11:57Z

@christophe-g I've seen all the work you've done and I'm really happy to see progress on this, but I'm not an owner of this repository and even if I would I'd like to wait for more people to take a look on this.

Unfortunately, it's kinda hard to reach any of the owners lately, I don't really know why. We have to wait for @tjmonsi or @mbleigh to have some time to take a look at this. I've even tried to tweet the Firebase team but got no response.

added exists attribute, reflecting whether we have data at a given pa…

844dcfd

…th. An "empty-result" event is fired by firebase-document or firebase-query to notify that the path does not hold any data.

christophe-g mentioned this pull request Jun 17, 2017

added an "exists" attribute to differentiate whether we have data or not #177

Closed

christophe-g mentioned this pull request Jul 5, 2017

Sorting & Getting Keys PolymerEl/firebase-nest#2

Closed

Merge branch 'master' into knowIfExists

5274aeb

christophe-g mentioned this pull request Sep 21, 2017

firebase-document defaults to {} before resolving #33

Open

christophe-g mentioned this pull request Oct 25, 2017

ready property for <firebase-document> and <firebase-query> #290

Closed

tjmonsi suggested changes Oct 28, 2017

View reviewed changes

merlinnot suggested changes Jan 8, 2018

View reviewed changes

christophe-g and others added 6 commits January 13, 2018 12:13

integrate @merlinnot comments

f6b0b14

integrate @merlinnot comment

58ed184

integrate @merlinnot comments

c8e301c

Merge branch 'master' into knowIfExists

ac153f4

Merge branch 'master' into knowIfExists

ab634d9

added tests

7290611

tjmonsi merged commit 6ca00b6 into FirebaseExtended:master Feb 21, 2018

Conversation

christophe-g commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christophe-g commented Jul 20, 2017

Uh oh!

JosefJezek commented Jul 24, 2017

Uh oh!

tjmonsi commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christophe-g commented Sep 7, 2017

Uh oh!

tjmonsi left a comment

Choose a reason for hiding this comment

Uh oh!

christophe-g commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjmonsi commented Oct 31, 2017

Uh oh!

andrewspy commented Oct 31, 2017

Uh oh!

christophe-g commented Oct 31, 2017

Uh oh!

tjmonsi commented Oct 31, 2017

Uh oh!

merlinnot commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christophe-g commented Jan 8, 2018

Uh oh!

merlinnot Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

merlinnot Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

merlinnot Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

merlinnot Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

merlinnot Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

merlinnot Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

christophe-g Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

merlinnot commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christophe-g commented Jan 13, 2018

Uh oh!

christophe-g commented Jan 23, 2018

Uh oh!

merlinnot commented Jan 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christophe-g commented Jun 17, 2017 •

edited

Loading

tjmonsi commented Sep 6, 2017 •

edited

Loading

christophe-g commented Oct 31, 2017 •

edited

Loading

merlinnot commented Jan 5, 2018 •

edited

Loading

merlinnot commented Jan 8, 2018 •

edited

Loading