Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solving memory leak in useTracker #383

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

Grubba27
Copy link
Contributor

@Grubba27 Grubba27 commented Feb 2, 2023

Video for reproducing the issue as shown in #382

Screen.Recording.2023-02-02.at.15.54.46.mov

closes #382
closes #381
closes #351

@Grubba27 Grubba27 merged commit 2846237 into master Feb 2, 2023
@CaptainN
Copy link
Contributor

CaptainN commented Feb 2, 2023

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
Copy link
Contributor Author

Grubba27 commented Feb 2, 2023

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
Copy link
Contributor

CaptainN commented Feb 2, 2023

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
Copy link
Collaborator

radekmie commented Feb 3, 2023

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
Copy link
Contributor

CaptainN commented Feb 3, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants