Open bimusiek opened 2 months ago
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundationβs CLA. Our records show that CLA has not been signed by @bimusiek. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check
comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.
@cla-bot check
The cla-bot has been summoned, and re-checked this pull request!
Hey @bimusiek thank you so much for the contribution. We'll try and get this reviewed shortly, but in the meantime, I was wondering if you have considered an approach similar to C#'s cancellation tokens - namely setting a flag that can be checked in the setImmediate
callback that would skip the listener registration altogether. My understanding is that your use case involves rapid registrations/unregistrations and I can imagine there might be a moderate performance gain if we avoid the work altogether. JS is not my strongest language, but I'm thinking something that looks roughly like this:
let registrationState: "registered" | "torn-down" | "pending" = "pending";
// ...
if (realm.isInTransaction) {
setImmediate(() => {
if (registrationState == "pending") {
collection.addListener(listenerCallback, keyPaths);
registrationState = "registered"
}
});
}
// ...
const tearDown = () => {
// ...
if (registrationState == "registered") {
collection.removeListener(listenerCallback);
}
registrationState = "torn-down";
};
@nirinchev This is good idea, I will implement it soon.
We have release the app to production with this change and it helped a lot. No more memory leaks due to listener callbacks leaks. However, the impact of this change was lesser than we thought, as we had another hook that was re-rendering a lot. Which triggered this scenario of leaking listener callback.
So at the end, this issue should not have huge impact.
The re-rendering issue we had, was related to Realm.BSON.ObjectID
which is changing reference even if the hexString
value did not change. I would like to open discussion about making ObjectID
cached so React won't rerender components if you use ObjectID
inside the deps
. Unfortunately this looks like deeper Realm integration and I have no idea how to approach this.
For now, we have patched react typings to disallow ObjectID
in the deps
so Typescript notifies us if someone uses ObjectID
directly. And we rewrote most of the code to operate on hexString
. But this seems like a huge oversight if Realm wants to integrate nicely with React.
@bimusiek, we've added your ObjectId
cache suggestion in this ticket. Feel free to add anything else you think is relevant.
What, How & Why?
If Realm is inTransaction and listener is created in the
setImmediate
, it won't be removed from listeners if tearDown is called quickly (which happens often if one listener triggers re-render of other listeners)This closes https://github.com/realm/realm-js/issues/1099#issuecomment-1998188175
βοΈ ToDos
Compatibility
label is updated or copied from previous entryCOMPATIBILITY.md
package.json
s (if updating internal packages)Breaking
label has been applied or is not necessary