search

realm / realm-js

Realm is a mobile database: an alternative to SQLite & key-value stores
https://realm.io
Apache License 2.0
5.62k stars 558 forks source link

Cannot remove a result object's listener #1597

Closed andrew-wagner89 closed 5 years ago

andrew-wagner89 commented 6 years ago

Goals

I'm trying to update my UI based on changes in to a realm object, so I added a listener to the result that calls forceUpdate. This solution, while a little ugly since I don't like calling forceUpdate, works correctly, but when I call removeListener on the collection, it still continues to call forceUpdate. I've also tried calling removeListener on the entire realm, as well as removeAllListeners on the result object and entire realm with no success.

I know its a no - op so it doesn't affect the app, but it is a nuisance and I feel like there should be a correct way I haven't found

Expected Results

the listener is removed correctly

Actual Results

it isn't and a No-Op react native warning is thrown due to react trying to update an unmounted component

Steps to Reproduce

add a listener to a result object in componentWillMount and remove it in componentWillUnmount

Code Sample

In the file where i define my realm instance I have the following: 

const listeners = [];

export function addFineGrainListener(objectName, filter, callback) {
  const collection = realm.objects(objectName).filtered(filter);
  collection.addListener(callback);
  listeners.push({ objectName, filter, collection, callback });
}

export function removeFineGrainListener(objectName, filter) {
  const listenerObject = listeners.find(
    listener => listener.objectName === objectName && listener.filter === filter,
  );

  listenerObject.collection.removeListener(listenerObject.callback);
}

and my component has the following:

  componentWillMount() {
    addFineGrainListener(
      'JournalEntry',
      \`id ==[c] "${this.props.screenProps.noteId}"\`,
      this.updateUIDueToRealmChange,
    );
  }
  componentWillUnmount() {
    removeFineGrainListener(
      'JournalEntry',
      \`id ==[c] "${this.props.screenProps.noteId}"\`,
    );
  }

  updateUIDueToRealmChange = () => {
    this.forceUpdate();
  };

Version of Realm and Tooling

kneth commented 6 years ago

Do you have the possibility to verify that listenerObject.callback is the same function/object as you push in addFineGrainListener?

andrew-wagner89 commented 6 years ago

Thanks for the quick response! Since I store the callback in the listeners array in addFineGrainListener and access it from the same array in removeFineGrainListener, its the same function, so I don't think that's the issue...

kneth commented 6 years ago

I am not saying that our tests are perfect but maybe it can help you by studying our test code:

jmparsons commented 6 years ago

Not being able to clean up collection listeners is the most critical bug for me. I've had to hack using listeners directly on realm instead of collections. It's terrible since there's exponentially more callbacks fired from it. It's as bad as the issues with addListener - https://github.com/realm/realm-js/issues/927.

Simple test - create collection:

test = (collection, changes) => {
  console.log('BAD');
};

track = () => {
  const wines = this.realm.objects('Wines');
  wines.addListener(this.test);
};

untrack = () => {
  const wines = this.realm.objects('Wines');
  wines.removeListener(this.test);
};

There's no way to remove the listener. This works:

test = () => {
  console.log('why cant we listen to collection');
};

track = () => {
  this.realm.addListener('change', this.test);
};

untrack = () => {
  this.realm.removeListener('change', this.test);
};

But it'll fire for anything that changes, so forcing update or setting state will throw errors, and not being able to clean up mounted components with listeners on collections will leak all over the place.

jmparsons commented 6 years ago

removeAllListeners does nothing as well on collections.

jmparsons commented 6 years ago

@andrew-wagner89 If you're using redux you can do this hack:

wines.addListener((collection, changes) => {
  const { insertions, deletions, modifications } = changes;
  if (
    getState().wines.tracking &&
    (insertions.length || deletions.length || modifications.length)
  ) {
    dispatch(wineUpdate());
  }
});

Where wines is a reducer and tracking is a property you toggle on it. At least then even if it doesn't get removed, you can intercept its non mounted call.

ZionChang commented 6 years ago

removeAllListeners not working

gunnigylfa commented 6 years ago

I am having the same problem we are not able to remove our listeners. In the [api docs] (https://realm.io/docs/javascript/2.2.0/api/Realm.Collection.html#removeListener) for the function removeListener it only specifies it to receive a callback as a parameter. But in the tests where the function is being used it is shown receiving two parameters. So the usage in the tests is: realm.removeListener('change', secondNotification); But according to the API docs the usage should be: realm.removeListener(secondNotification);

We are using it like the api specifies it but that is not working for us but if we try to pass the string 'change' as well (i.e. calling the function with two parameters the type of subscription and the callback assigned to the listener) we get an unhandled Promise Rejection because too many parameters are being given to the function.

Which is the correct usage of the add/removeListener api. Like the tests tests suggest or the way the [api docs do] (https://realm.io/docs/javascript/2.2.0/api/Realm.Collection.html#removeListener)?

ZionChang commented 6 years ago

Finally, I think it would work

componentDidMount() {
    this.results = this.realm.objects('ResourceDownloadInfo')
}

componentWillUnmount() {
    if (this.results) {
        this.results.removeAllListeners()
    }
}
kneth commented 6 years ago

@gunnigylfa There is a difference between Realm.addListener()/Realm.removeListener() and Realm.Results.addListener()/Realm.Results.removeListener()`.

FlaviooLima commented 6 years ago

Hi, I'm having the same problem. What am I doing wrong? From the previous posts, I see a few people are mistaking Realm Notifications with Collection Notifications. How can I remove Collection Notifications Listeners, because realm.removeAllListeners() doesn't work as it says in the documents. btw shouldn't we be able to remove a single listener for Collection Notifications?(Didn't see this in the Docs, or is the same as Realm Notifications?) At this point, I'm a bit confused and I don't know if this is a bug or a bad interpretation of me.

 realm.objects("Users").addListener(this.updateUI); // triggers the function when changes are made

 realm.removeAllListeners(); // dont work on above line but works on (Realm.addListener() from Realm Notifications)

 realm.objects('UsersInfo').removeAllListeners(); // dont work  but i dont know if this is possible right now

 realm.objects('UsersInfo').removeListener(this.updateUI); // dont work  but i dont know if this is possible right now

System Info: iOS : 11 realm: "2.2.5" react-native: "0.44.1"

----EDIT---------------------------------------------------------------------------------------- After re-reading this entire post a couple of times I saw @kneth says in the first post, and I tried the fowling and it works:


var user =  realm.objects("Users");
user.addListener(this.updateUI);
user.removeAllListeners(); // removes all listeners in the user (realm.objects("Users"));
kneth commented 6 years ago

Of course I will claim that our documentation is crystal clear but when reading the many comments in this issue it doesn't seem to be (in particular the API documentation could use some love).

FlaviooLima commented 6 years ago

@kneth I don't believe this code in the docs works : 

// Observe Collection Notifications
realm.objects('Dog').filtered('age < 2').addListener((puppies, changes) => {

  // Update UI in response to inserted objects
  changes.insertions.forEach((index) => {
    let insertedDog = puppies[index];
    ...
  });

  // Update UI in response to modified objects
  changes.modifications.forEach((index) => {
    let modifiedDog = puppies[index];
    ...
  });

  // Update UI in response to deleted objects
  changes.deletions.forEach((index) => {
    // Deleted objects cannot be accessed directly
    // Support for accessing deleted objects coming soon...
    ...
  });

});

// Unregister all listeners
realm.removeAllListeners();

Could you test ? Below is my test, maybe I'm doing it wrong.


realm.objects('info').filtered('del == 1').addListener((puppies, changes) => {
      alert('changed');
    });

    realm.removeAllListeners();

    realm.write(() => {
      realm.create('info', { id: 1000, del: 1, updated: "string" }, true);
  });
gunnigylfa commented 6 years ago

@kneth Thank you for responding. I did not make the distinction while viewing the test file. On the other hand the problem still persists for us.

To clarify we are fetching a collection like the dogs collection like so let dogs = realm.objects('Dog') we get a Results from that one right? dogs.addListener(watchDogsCallBack); So having done that I'm unable to remove this callback after trying the following methods:

FlaviooLima commented 6 years ago

@gunnigylfa Strange, the same happen to me, but it was my fault. This code of yours should work: dogs.removeAllLIsteners() can you do a test for me? Put the dog variavel initializeing globally like this:


var dogListnerGlobal;
export default class BlaBlaBlaComponent extends React.Component {
  constructor(props) {
    super(props);
    dogListnerGlobal = realm.objects('Dog');
    dogListnerGlobal.addListener(this.watchDogsCallBack);
  }

  watchDogsCallBack = () => {
    alert("something has change");
  }

  componentWillUnmount = () => {
    dogListnerGlobal.removeAllListeners();
  }

}
gunnigylfa commented 6 years ago

I think my problem has something to do with the way I was updating the component state in the function I assigned to the listener. I was fetching a new version of the objects and calling setState with those newly fetched items rather than using those passed to me via the callback. I'm not sure why that would be the case. I'm working on refactoring my code in order to make the new approach work for me. @FlaviooLima Thank you but I'm not sure if I can use this approach since I'm writing this in a type safe language which compiles to JS.

I will post an update of the problem here once I'm able to replicate with the new approach.

HZSamir commented 6 years ago

Thank you @FlaviooLima It works. This should really go somewhere on the anemic documentation. Though this issue was the last straw for me, I doubt I'll ever use Realm in another project, at least for the foreseeable future.

kneth commented 6 years ago

It sounds more like a documentation issue to me. I will relabel it as such, and try to get some time in the near future to update the documentation.

FlaviooLima commented 6 years ago

@kneth Maybe like this?

this.collectionListenerRetainer = realm.objects('Dog').filtered('age < 2');

// Observe Collection Notifications
this.collectionListenerRetainer.addListener((puppies, changes) => {

  // Update UI in response to inserted objects
  changes.insertions.forEach((index) => {
    let insertedDog = puppies[index];
    ...
  });

  // Update UI in response to modified objects
  changes.modifications.forEach((index) => {
    let modifiedDog = puppies[index];
    ...
  });

  // Update UI in response to deleted objects
  changes.deletions.forEach((index) => {
    // Deleted objects cannot be accessed directly
    // Support for accessing deleted objects coming soon...
    ...
  });

});

// Unregister all listeners
realm.removeAllListeners();

// OR Unregister this listener
this.collectionListenerRetainer.removeAllListeners();
kneth commented 6 years ago

Updated for the 2.6.0 documentation

jfr3000 commented 5 years ago

The docs now say 

// Unregister all listeners
realm.removeAllListeners();

// OR Unregister this listener
collectionListenerRetainer.removeAllListeners();

which doesn't look accurate to me, because the second example still removes all listeners from the collection. Is it correct that there is no way to remove just one listener from a collection? Because that is very unfortunate.

FlaviooLima commented 5 years ago

@jfr3000 Does this code removes every listner from every collection? I'm sorry but I don't have my code right now, and I don't remember correctly, but it should only remove the listeners from the "collectionListenerRetainer" variable.

// OR Unregister this listener
collectionListenerRetainer.removeAllListeners();
jfr3000 commented 5 years ago

@FlaviooLima no, it only removes all listeners from the collection in question, not from all collections (I hope, didn't test that). but I just found out, collection.removeListener(listenerFunc) also works, it's just not documented?

FlaviooLima commented 5 years ago

@jfr3000 Sorry I understand it wrong :) It's documentd. But from what you are saying that is usual javascript behavior in relation to listeners, maybe the devs take it into account when creating. I'm inclined to say that you are correct and that that example is missing from the main documentation, but not missing from the API Docs. :) Were you able to do that code without passing the "name" parameter, just the function?

jfr3000 commented 5 years ago

Yes, just the function. It complains when given 2 arguments. But that's how it's actually documented here now that you point me that way. Thanks!

FlaviooLima commented 5 years ago

@jfr3000 I just realized that I point out in a half wrong direction xD, sorry about that. glad that you found the correct way :) I believe that not all the people realize that you have two sections: realm docs AND realm API. Should we improve this in some way?

jfr3000 commented 5 years ago

I guess I had realized that the two exist, but sometimes wasn't sure that the API docs were up to date and also didn't realize that the realm docs actually aren't complete. Not sure if a link a the end of each section For full reference check the API docs on this would be overkill? In any case, I think

// OR Unregister this listener
collectionListenerRetainer.removeAllListeners();

is not super accurate, because it doesn't just unregister this listener but all of them. It led me to believe that the only listener removal method for collections is removeAllListeners. Just my 2 cents, thanks for clarifying this!

FlaviooLima commented 5 years ago

@jfr3000 can you build a more accurate example to post it on the docs? @kneth Do you agree? if yes maybe another update to the docs section?

jfr3000 commented 5 years ago

you'd have to name the listener function, maybe like this

collectionListenerRetainer = realm.objects('Dog').filtered('age < 2');

function onPuppiesChange(puppies, changes) {

  // Update UI in response to inserted objects
  changes.insertions.forEach((index) => {
    let insertedDog = puppies[index];
    ...
  });

  // Update UI in response to modified objects
  changes.modifications.forEach((index) => {
    let modifiedDog = puppies[index];
    ...
  });

  // Update UI in response to deleted objects
  changes.deletions.forEach((index) => {
    // Deleted objects cannot be accessed directly
    // Support for accessing deleted objects coming soon...
    ...
  }

// Observe Collection Notifications
collectionListenerRetainer.addListener(onPuppiesChange);

// Unregister all listeners from collection
collectionListenerRetainer.removeAllListeners();

// OR Unregister this listener
collectionListenerRetainer.removeListener(onPuppiesChange);
FlaviooLima commented 5 years ago

Looks good :) @kneth is it possible? do you agree? :)

kneth commented 5 years ago

@jfr3000 @FlaviooLima It should work (and it is similar to our test: https://github.com/realm/realm-js/blob/master/tests/js/async-tests.js#L76).

kneth commented 5 years ago

I have change the documentation a bit. It will be online soon.