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

Terrific memory leaks on remounting #29

Closed
vintlucky777 opened this issue Dec 11, 2015 · 13 comments
Closed

Terrific memory leaks on remounting #29

vintlucky777 opened this issue Dec 11, 2015 · 13 comments

Comments

@vintlucky777
Copy link

Memory leaks on frequently remounting components

In the case when we use a react-gmaps, say, in kind of "details-view" component, that will mount and unmount frequently, gmaps lead to huge memory leaks.

The good way would be to use a single instance of gmaps, accociated with DOM id of the parent element.

Moreover to that, to support hot-reloading, this instance can be stored in the window scope, and taken from there on component mount

@vintlucky777
Copy link
Author

I've actually already been solving an issue like that in a project of mine. But I used native gmaps js API.
Should I make a PR to fix this?

@MicheleBertoli
Copy link
Owner

Hey @vintlucky777,
thank you very much for pointing out the problem.

Personally I never had any memory-related problems with the Google Maps in my applications but I found the issue.

If you could submit a PR it would be great, in the meanwhile I'll try to reproduce it.

Have a nice day!

@MicheleBertoli
Copy link
Owner

Here we go:

screen shot 2015-12-13 at 13 29 27

[source](https://gist.github.com/MicheleBertoli/75de5da8fd625b51c03c)

@MicheleBertoli
Copy link
Owner

Any updates on this @vintlucky777?
Thanks!

@vintlucky777
Copy link
Author

@MicheleBertoli I still find the only solution to actually work is the one when you manually keep the references to gmaps DOM-node and try to reutilize it as much as possible.
I'll put up a PR for this one.

@dstockdale
Copy link

Hi @vintlucky777 / @MicheleBertoli. Any progress with this issue? I've come up against this happening as well now. I'll go ahead and have a look at what the issue is but don't want to redo anything that's already been sorted out.

@MicheleBertoli
Copy link
Owner

Hey @dstockdale, I'm really interested in fixing the issue as well.
I was waiting for @vintlucky777 because he said he already had a solution.
If I won't receive any PR in the next days, I'll fix it.
Have a nice day!

@MicheleBertoli
Copy link
Owner

Hello @dstockdale and @vintlucky777,
I spent a few hours working on the memory issue and I've got some pretty good results to share.

Running this script 100 times on the current master gives the following results:

screen shot 2016-03-28 at 17 26 44

While running it on the memory-leak branch gives this:

screen shot 2016-03-28 at 17 25 26

I still want to clean the code up a little bit before merging and publishing the new version on npm but I'm pretty happy with the solution.
Feedbacks are welcome :)

@dstockdale
Copy link

Hi @MicheleBertoli sorry I've been having a bit of a nightmare with a project going gnarly on me but looks good. I'll test it out tomorrow and let you know how what I was working on runs with it.

@MicheleBertoli
Copy link
Owner

Thank you very much @dstockdale, you can install it with npm i react-gmaps@rc.
I'm now happy with the code and the memory leak is fixed.

The only problem is that when you recycle a previously created map, you get a map with all the options you applied before. For example, if you set a custom style or you hide some controls you are going to get a map with those styles and controls all the time.
I (partially) solved the problem applying the "default" options before reusing the map but if a user added some overlays directly on it (using onMapCreated instead of the lib's components) there's no way to clean them out.
That's the reason why I didn't merge it yet.

@sonsoleslp
Copy link

Is this fixed in the master branch now?

@MicheleBertoli
Copy link
Owner

Hello @sonsoleslp, this branch has never been merged because it needed proper testing and feedback, but please keep in mind that the "memory leaking" problem is due to the Google Maps SDK (not this package) and it happens only in some particular scenario (for example, it never happened to me - and I used this packaged in production).

@MicheleBertoli
Copy link
Owner

Closing due to inactivity.

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

No branches or pull requests

4 participants