Allow differentiation between no data and data not yet loaded in the array case

[jwngr]

First crack at fixing this was in #73.

[joenoon]

@jwngr: I've put two options together to get your thoughts.

The first (#75) is what you mentioned in #73. Unfortunately its still necessary to have the value handler, since if there is no data at the path, none of the child* events will fire, and we wouldn't be able to differentiate between no data and data not loaded.

The second (#76) is a more extreme deviation that might at least be a good discussion. I'm not sure there is much gained from maintaining the array manually. Its definitely neat, but the firebase lib is probably already doing this internally as part of the value event. We need the value event, so the question really is: is one value event that re-runs _createRecord on each child every time it changes more efficient than 4 child events that manually manipulate the array.

And to take that a step further, this really made me wonder about a change that would be totally backward incompatible: If we got rid of _createRecord completely, and simply expose the Firebase.DataSnapshot objects directly, a lot of these problems go away:

• data not yet loaded: !bindVar
• no data: bindVar.val() == null
• key: bindVar.key()
• bindAsArray and bindAsObject could be combined into one
• the user could iterate an array with snapshot.forEach

I suspect this would be the most efficient way, since:

• it relies on whats already done internally in the firebase lib
• it minimizes setState calls
• the developer is in full control of what they need to access/iterate during render

Interested to get your thoughts on this and if you've already ruled some of these ideas out for various reasons.

[jwngr]

Joe, thanks for this. Sorry I haven't gotten around to it yet. I hope to do so at some point this week.

[joenoon]

@jwngr Please feel free to close these. We've since started to do what I was describing last above, where we just basically have a bindSnap mixin, and thats the extent of it. That lets us check if its present, iterate it as an array, or use it as a value. This is working out well for us so far.

[jwngr]

Thanks Joe. Glad you got something working! Sorry for never responding to all of these. None of them really fit into the Firebase model that well unfortunately without making some breaking changes which I don't really want to do right now. Closing for now, but will reconsider these in the future.

[tirsen]

Any progress on this? In particular I would like two things: 1) differentiate between loaded/no data, 2) some way of knowing if all data has been loaded yet. The latter is to be used for e.g. server side rendering or automated tests.

[tirsen]

I think the best way to work around the backwards compatibility issue is to just have an extra "bindAsDataSnapshot" method. That would address my first request, but not my second.

[jwngr]

For posterity sake, here are my comments on the two PRs @joenoon put together since I never initially responded to them directly:

#75 - I think the code here is good although I'm not wild about the var array = this.data[bindVar] || (this.data[bindVar] = []); syntax. I also wonder if it will be confusing / unexpected that the array bound to this.state can be undefined (aka, not an array) before we get the initial data. This does allow you to explicitly differentiate between no data and data not yet loaded, but it also means you are forced to handle the undefined case. I'm wondering if we are actually making things more difficult for all developers to handle a specific use case.

#76 - This is actually the short term solution I proposed in #36. It most definitely works and simplifies the code, but it does so at the expense of performance, especially for large lists. If you have thousands of records, recreating the array and calling val() so many times results will just kill your app.

---

@tirsen - I'm definitely not against making breaking changes if they make sense and simplify the API. I just am not sure I like any of the proposed solutions to this problem. What you really want to do is what I suggest in this Stack Overflow post. Adding a value event with the regular Firebase SDK is not ideal, but it does give you intended behavior with little extra code:

var ExampleComponent = React.createClass({
  mixins: [ReactFireMixin],

  getInitialState: function() {
    return {
      items: [],
      itemsLoaded: false
    };
  },

  componentWillMount: function() {
    const rootRef = new Firebase("https://<YOUR-FIREBASE-APP>.firebaseio.com");
    const itemsRef = rootRef.child("items");

    // Bind existing and future items to this.state.items
    this.bindAsArray(itemsRef, "items");

    // The "value" event will fire after any existing items will be bound to this.state.items
    itemsRef.on("value", () => {
      this.setState({
        itemsLoaded: true
      });
    });
  },

  render: function() {
    const { items, itemsLoaded } = this.state;    

    // If the items array is empty... 
    if (items.length === 0 && !itemsLoaded) {
      if (itemsLoaded) {
        // .. and Firebase has loaded the initial data, there really are no items
        return <p>No items!</p>;
      } else {
        // ... but Firebase hasn't loaded the initial data yet, show a loading screen
        return <p>Loading...</p>;
      }
    });

    // Otherwise, display the items
    return <pre>{ this.state.items }</pre>;
  }
});

---

As I think about this more, @joenoon's initial suggestion in #73 to add some sort of firebaseLoads variable to automatically track when the value event fires (so you don't have to add that block of code yourself) is not a bad idea. I just dislike the need for that variable and it seems to go a bit against the Firebase philosophy which suggests you treat initial and new data as one and the same.

---

Alright, I've written enough about this for now. I'll re-open this issue and see if we can come to a sensible solution together.

So, what do you think?

[tirsen]

I don't think you necessarily need to make breaking changes. You can add a new "bindAsDataSnapshot" method in addition to other two binding types.

That said, I've been building rather complex apps with this for a few months now and I do think you need to make some breaking changes. In particular I think the bind methods need to be idempotent: everything else in React is idempotent, when one of your key pieces of infrastructure isn't then things becomes difficult. I had to build idempotency on top of Reactfire and that was getting quite tedious.

I've made a fork where I've made some changes necessary to support the app I'm working on. I think the API isn't there yet but in terms of functionality I think this is stuff that is necessary.
https://github.com/tirsen/reactfire

In terms of API I think it needs a complete overhaul. Mixins are (slowly) getting phased out and replaced by higher order components. I would take inspiration by Relay which is the "modern" way of building React extensions and accomplishes something very similar to Reactfire (although Firebase makes the whole thing both more productive and more powerful).
https://facebook.github.io/relay/

[jwngr]

Thanks for the feedback. Here are some comments:

You can add a new "bindAsDataSnapshot" method in addition to other two binding types.

I don't like the API of that as I think it will lead to the bad practice of calling val() on the DataSnapshot multiple times which can be really bad for performance.

I think the bind methods need to be idempotent... I had to build idempotency on top of Reactfire and that was getting quite tedious.

I'm not sure I understand what you are suggesting here. How does this relate to the issue at hand? Also, in 99% of cases you only need to call bindAs*() once. It's possible I'm just misunderstanding what you are suggesting. Can you please explain?

Mixins are (slowly) getting phased out and replaced by higher order components.

Yeah I've been meaning to look into an HOC for ReactFire for a while now but just don't have the time. See #38 for some discussion on this. I think this point is orthogonal to this particular issue though.