invertase / react-native-firebase

🔥 A well-tested feature-rich modular Firebase implementation for React Native. Supports both iOS & Android platforms for all Firebase services.
https://rnfirebase.io
Other
11.7k stars 2.21k forks source link

Listener callback only triggers for initial component instance #25

Closed AndrewHenderson closed 7 years ago

AndrewHenderson commented 7 years ago

@Salakar I'm attempting to use the pattern you referenced in a previous issue.

The Problem

In my iOS app, it's possible to have multiple instances of a component mounted at the same time.

Think Instagram – when a user taps a photo in Instagram and then taps the photo owner's avatar, the user scene pushes in from the right.

If she then goes on to tap the same photo in the user's list of photos, a clone of the first scene will also push in the from right.

In my app, all scenes need to mount, establish the Firebase listener, get a snapshot of the initial data and listen for changes after that.

Everything works great for the first component mounted. In any subsequent components, however, the listeners don't immediately trigger the callback.

I suppose makes sense considering the docs:

Firebase data is retrieved by attaching an asynchronous listener to a firebase.database.Reference. The listener is triggered once for the initial state of the data and again anytime the data changes.

It's as though Firebase sees each subsequent listener as the same listener and thus doesn't trigger.

Data Structure

In my code, I'm following Firebase's recommended data fan-out approach which seeks to create "data that scales."

I've included the following JSON, so the complexity of the code that follows will be more clear.

{
  "displayName" : "Andrew Henderson",
  "host" : {
    "-KgC-c0WvT0P1R-aDmVo" : true,
    "-KgC0D94Cxs05GX8qWBc" : true,
    "-KgC0s7f9lwweeDm3oeg" : true,
    "-KgC0ypAODARXxvbva5F" : true
  },
  "puid" : "-Kg6pefxyFtyfPVe-aIo",
  "timestamp" : 1490485162946,
  "uid" : "H63dvD6TvJRZnpPwf9o0FejqmEf2"
}

Code

In my component, I set up a reference to a user's posts which returns the host object.

I then create a reference to all of the individual posts using those keys and receive the values for the full posts.

constructor(props) {

  super(props);

  const { uid } = props;

  // Set firebase refs
  this.postsRef = firebase.database().ref('posts');
  this.hostsRef = firebase.database()
    .ref(`users/${uid}/host`)
    .orderByChild('startTime');

  this.hostedPostRefs = {};

  // Keep a raw copy of this user's posts
  this.hostedPosts = {};

  // ListView DataSource instance
  this.hostedPostsDataSource = new ListView.DataSource({
    rowHasChanged: (r1, r2) => r1._key !== r2._key,
  });

  // Initial State
  this.state = {
    loading: true,
    hostedPostsDataSource: this.hostedPostsDataSource.cloneWithRows(this.hostedPosts),
  };
}

componentDidMount() {
  this.hostsRef.on('value', this._onHostsReceived)
}

componentWillUnmount() {
  this.hostsRef.off();
  forEach(this.hostedPostRefs, hostedPostRef => {
    hostedPostRef.off()
  })
}

_onHostsReceived = (snapshot) => {
  snapshot.forEach(this._requestPost)
};

_requestPost = (snapshot) => {

  const { key } = snapshot;

  if (!this.hostedPostRefs[key]) {
    this.hostedPostRefs[key] = this.postsRef.child(key);
    this.hostedPostRefs[key].on('child_changed event', this._onPostReceived)
  }
};

_onPostReceived = (snapshot) => {
  this.hostedPosts[snapshot.key] = snapshot.val();
  this.setState({
    hostedPostsDataSource: this.hostedPostsDataSource.cloneWithRows(this.hostedPosts)
  });
};
Ehesp commented 7 years ago

This all comes down to the general implementation of the database setup. Once upstream messaging & android storage are fixed this will most likely be the next thing to focus on. Database needs a big overhaul/cleanup anyway.

AndrewHenderson commented 7 years ago

This all comes down to the general implementation of the database setup

@Ehesp My particular database or Firebase's implementation of databases?

Once upstream messaging & android storage are fixed this will most likely be the next thing to focus on. Database needs a big overhaul/cleanup anyway.

Whose workload is this? I'm not familiar with these issues and how they relate to my issue.

If it matters, this is an iOS only app at the moment.

Salakar commented 7 years ago

My particular database or Firebase's implementation of databases?

@AndrewHenderson RNFirebase implementation

Whose workload is this? I'm familiar with these issues and how they relate to my issue.

Mine 🙈 - have just landed android storage a few minutes ago, have just started upstream messaging and I've planned my Friday night to overhaul database , improve its performance and fix this issue (feature Friday ;p, I spend every Friday night till stupid o'clock hammering out a new feature or an implementation of something, last Friday was transactions).

AndrewHenderson commented 7 years ago

@Salakar OK. Looking forward to the update!

I'm still relatively new to React Native and iOS development. It seems a lot of packages such as this one seek to make the Web version of a package friendly to React Native or Native itself.

It seems the general approach is to expose a bridge to a Native module and have the JS thread communicate across that. I believe there's a performance benefit to offloading the work to the Objective-C code rather than keeping it in the JS thread, is that correct?

On a high level, I grasp these concepts. However, I want to better understand how one goes about creating these packages as well as contributing. I assume one needs to learn either Swift or Objective-C. I'm currently learning Swift.

Also, with this package are we chasing a moving car, trying to maintain parity with the firebase features and API? That is one of the big selling points for me and why I came to this lib from react-native-firestack.

That said, I have concerns that this package's maintainers, like firestack's, will eventually tire of the chase and the issues pile up without any resolution in sight.

Salakar commented 7 years ago

t seems the general approach is to expose a bridge to a Native module and have the JS thread communicate across that. I believe there's a performance benefit to offloading the work to the Objective-C code rather than keeping it in the JS thread, is that correct?

Correct.

In the case of firebase, google also provide native sdk's for ios and android, which is what we use on the native side, therefore it's just a case of mapping what the sdk functions has to the equivalent firebase native sdk function on ios or android, with a few things like data type conversions and error code/messages conversions inbetween. There's also some special cases where the web sdk has no implementation of a native feature, for example, firebase native sdks have crash reporting and analytics but the web sdk doesn't.

I assume one needs to learn either Swift or Objective-C. I'm currently learning Swift.

As far as I know react native modules ios wise are all objective-c based, don't take my word on that though. Android wise it's Java.

Also, with this package are we chasing a moving car, trying to maintain parity with the firebase features and API?

No I don't think so, obviously with the first release it's playing catch up to existing functionality, thereafter it's shouldn't be as much to do.

If so, I have concerns that this package's maintainers, like react-native-firestack's, will eventually tire of the chase and the issues pile up without any resolution in sight.

Tell me about it, we used firestack, eventually I ended up writing most of the v3 version of firestack before giving up with the lack of project maintenance from its owners and started this one. We use this in production apps so I don't see this getting abandoned, also have a busy year ahead in terms of new apps that will make use of this, so keeping it up to date is in my best interest 🙈

AndrewHenderson commented 7 years ago

@Salakar Any progress on this issue? It's kind of a "show stopper" for me.

I can imagine some workarounds, using Redux caching. However, I'd really like to avoid writing all that extra code if this fix is coming soon.

chrisbianca commented 7 years ago

This should be resolved as of 1.0.0-alpha13. Please let us know if you're still having issues once you've updated.