search

meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
571 stars 157 forks source link

Solving memory leak in useTracker #383

Closed Grubba27 closed 1 year ago

Grubba27 commented 1 year ago

Video for reproducing the issue as shown in #382

https://user-images.githubusercontent.com/70247653/216427576-11237efd-bcc9-468d-812e-3aa61273ecdb.mov

closes #382 closes #381 closes #351

CaptainN commented 1 year ago

I actually tried this before, with no luck. I wonder if this would break the implementation on older versions of react?

Since you are stopping the computation in-line, you no longer need the Meteor.defer block within the same Memo block. That should probably be removed.

Grubba27 commented 1 year ago

I wonder if this would break the implementation on older versions of react?

I wonder why it would. I can give it a try.

Since you are stopping the computation in-line, you no longer need the Meteor.defer block within the same Memo block. That should probably be removed.

I thought that having both of them sounded safer (I'm not sure why I thought that), probably because it stops the computation as possible, that being inline(first if) or when setTimeout is ran

CaptainN commented 1 year ago

You want to be careful about running that on a timeout though, because if a new one gets added to the ref before the timeout (which is always a minimum of 4ms, and I think longer in some cases with defer), you could end up cleaning up the wrong reference. (That SHOULDN'T happen, but it's not super clear and hard to test, and if you are already successfully stopping it, you might as well remove the uncertainty.)

As for why it would fail - I have no idea. Never figured it out, but I do know it showed in the tests.

radekmie commented 1 year ago

I'm glad my idea worked 😅 As for what @CaptainN said, I think we could also save this computation in a constant and then stop it in Meteor.defer. We may revisit it if someone reports an issue.

CaptainN commented 1 year ago

Some of the issue I may remembering, BTW, may be isolated to versions of IE - in which case, we probably don't have to worry about them any more. It's been long enough that I don't remember the details. :-)

But what @radekmie said makes sense - if you are going to destroy the computation in the same time slice, you don't need to store it in a ref at all, a local const would be ideal to avoid cross talk and leakage.