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

Added 'firebase-loaded' event to notify clients when initial data has… #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

johnlim
Copy link

@johnlim johnlim commented Apr 6, 2016

… been loaded and can be accessed.

Hi,

There are instances when a client needs to know when it is able to start accessing data from firebase. Having an event to signal that really helps.

Following the docs from firebase, this PR creates an event to signal the client when data from Firebase has been loaded and can be accessed.

@ebidel
Copy link
Contributor

ebidel commented Apr 12, 2016

Why can't clients use firebase-value and do the booking themselves?

@johnlim
Copy link
Author

johnlim commented Apr 13, 2016

@ebidel They could.... but in situations where the client needs to do something only once (i.e on initial connect to firebase), without the firebase-loaded event, the client will have to do something like this

.......
   <firebase-collection location="{{location}}"
                         data="{{data}}"
                         on-firebase-value="_firebaseLoaded">
    </firebase-collection>
.......
   <script>
    Polymer({

      is: 'custom-element',

      properties: {
         _skipFlag: {
          value: false
        }
      },
     _firebaseLoaded: function() {
       if(!this._skipFlag) {
         doInitialisationStuffLikeLinkPathsEtcOnFirstLoad(); 
         this._skipFlag = true;
      }
       doOtherStuffWhenDataChanges(); 
      }

This results in clients having to "manually" set/clear flags which is error prone and also having to duplicate these checks across the app (where it's needed).

Having the firebase-loaded event would help in writing 'cleaner' client code.

@ebidel
Copy link
Contributor

ebidel commented Apr 13, 2016

I agree it's a lot nicer to have the event and save some code writing. I'm just thinking about the cost of the listener (even a once listener) and firing an event for every users of firebase-element. The number of users that need the event might be very small compared to everyone that's using the element.

@johnlim
Copy link
Author

johnlim commented Apr 13, 2016

Hmmm... we could have something like this

    _listenToQuery: function(query) {
      this._log('Adding Firebase event handlers.');
//      query.on('value', this._onQueryValue, this._onQueryCancel, this);
      query.on('child_added', this._onQueryChildAdded, this._onQueryCancel, this);
      query.on('child_removed', this._onQueryChildRemoved, this._onQueryCancel, this);
      query.on('child_changed', this._onQueryChildChanged, this._onQueryCancel, this);
      query.on('child_moved', this._onQueryChildMoved, this._onQueryCancel, this);
      query.once('value', this._onInitialDataLoaded, this);
    },
 _onInitialDataLoaded: function(snapshot) {
      this.fire('firebase-loaded', snapshot, { bubbles: false });
      this.fire('firebase-value', snapshot, { bubbles: false }); //maintain backward compatibility
      this.query.on('value', this._onQueryValue, this._onQueryCancel, this);
    },

or we could move the flag and if checking into the behaviour itself like so

    _onQueryValue: function(snapshot) {
      this._log('Firebase Event: "value"', snapshot);
      if(this._onInitialDataLoaded) {
        this.fire('firebase-loaded', snapshot, { bubbles: false }); 
      }
      this.fire('firebase-value', snapshot, { bubbles: false });
    },

but it somehow feels ugly and the original suggestion seems cleaner.

@johnlim
Copy link
Author

johnlim commented Apr 17, 2016

@ebidel I've pushed an alternate proposal. Does that eliminate the concern you have?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants