[android][database] Remove all db references and listeners onCatalystInstanceDestroy

[Salakar]

Potential fix for #1498

cc @pistou , @barbarosh , @shashankvaibhav

[codecov]

Codecov Report

Merging #1535 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1535   +/-   ##
=======================================
  Coverage   70.15%   70.15%           
=======================================
  Files          71       71           
  Lines        1833     1833           
=======================================
  Hits         1286     1286           
  Misses        547      547

[barbarosh]

@Salakar Looks like a good solution, but I think we should have this same solution and for iOS ?

[Salakar]

@barbarosh I'd like someone to test and confirm this works before I look into iOS - I'm not even sure if iOS has the same issue - can someone confirm?

[barbarosh]

@Salakar onHostDestroy isn't calling on a restart js part in dev mode.

[Salakar]

@barbarosh have just checked and looks like that's an RN problem I think: facebook/react-native#9773

If someone could push up a cloneable repo that reproduces this it'd save me time and I can look into somehow working around it.

This change on this PR is most likely still be required either way.

[shashankvaibhav]

@Salakar You can use this method. It gets triggered when after hot reloading. I tried to use the code you wrote in onHostDestroy under this method but still no benefit.

@OverRide
public void onCatalystInstanceDestroy() {

}

[barbarosh]

@Salakar It can be useful as example https://github.com/gre/react-native-view-shot/blob/master/android/src/main/java/fr/greweb/reactnativeviewshot/RNViewShotModule.java#L53

[Salakar]

Am trying something else - will push to this branch for you guys to test, give me a few mins.

[barbarosh]

@Salakar I tried this code

  @Override
  public void onCatalystInstanceDestroy() {
    super.onCatalystInstanceDestroy();
    Iterator refIterator = references.entrySet().iterator();
    while (refIterator.hasNext()) {
      Map.Entry pair = (Map.Entry) refIterator.next();
      RNFirebaseDatabaseReference nativeRef = (RNFirebaseDatabaseReference) pair.getValue();
      nativeRef.removeAllEventListeners();
      refIterator.remove(); // avoids a ConcurrentModificationException
    }
}

It worked, bot I worried, it code can clearify new subscriptions while starting new JS instance ?

[shashankvaibhav]

Guys in this method removeAllEventListeners() thers is a if statement with condition hasListeners.
It gives me false value after hot reload. This is the reason it is not going inside and removing listeners.

[Salakar]

Can you locally replace the two files I've just changed again and let me know how you get on?

[shashankvaibhav]

@Salakar Sorry but didn't work. Same problem no data after hot reload.

[Salakar]

@shashankvaibhav can you comment out the highlighted lines below (the iterator section) and try:

[shashankvaibhav]

@Salakar Didn't work.

[Salakar]

Worked for @barbarosh though 🙈

Can you push up a reproducible project that I can clone and work off please to save me some time - and I can take a look

[armanatz]

@Salakar I tried this PR out and even commented out the lines you mentioned above. Still the same issue

[Salakar]

@armanatz ok thanks for confirming also - I'll try reproduce on my end - will take time to setup though, I've also not experienced this issue myself so am not 100% sure if this is even the cause.

[barbarosh]

@Salakar for me worked solution with onCatalystInstanceDestroy callback, but i tried only for reload event from dev menu, I not check with hot reload

[armanatz]

@barbarosh Interesting because this PR doesn't work for me with dev menu reload

[Salakar]

@barbarosh @armanatz what RN versions are you each using?

[armanatz]

@Salakar I am on [email protected] and [email protected].

[barbarosh]

@Salakar I am on [email protected] and [email protected].

[Salakar]

See new PR #1619