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

Fix chunckedLoading LatLngBounds.intersects() #891

Merged
merged 7 commits into from
May 18, 2018

Conversation

boldtrn
Copy link
Contributor

@boldtrn boldtrn commented May 5, 2018

This PR fixes #743. We use Leaflet.markercluser with chunckedLoading in production and have seen issues similar to the ones that are reported in #743.

We applied this fix over two months ago and we haven't seen the issue again eversince. Also see my comment in #743, why this fixes the issue.

@danzel
Copy link
Member

danzel commented May 6, 2018

Can we have a test case that reproduces this too please?
See the spec dir.
If that's a bit tough, then at least an example in example/old-bugs.
Thanks!

@boldtrn
Copy link
Contributor Author

boldtrn commented May 7, 2018

The problem with tests is that they fail locally similar to Travis, so I cannot really write a test without testing it :).

I created an example.

With c7932f6 you are able to reproduce the bug. With bc40326 it's fixed.

It is a bit hard to reproduce, you need to zoom out before the cluster is fully loaded in order to trigger the removeLayers before the chunckedLoading is done. It is important to do this during the zoom.

@danzel
Copy link
Member

danzel commented May 13, 2018

I've fixed the tests, please create a test instead of the example if possible!

@boldtrn
Copy link
Contributor Author

boldtrn commented May 14, 2018

I've fixed the tests, please create a test instead of the example if possible!

Awesome work!

I created a test and showed that it failed and that the code change fixed the failure.

I am not sure how many markers are needed in order to provoke this issue. I settled for 1000, but I think it might be possible to further reduce it.

@danzel danzel merged commit 9b45d26 into Leaflet:master May 18, 2018
@danzel
Copy link
Member

danzel commented May 18, 2018

Great thanks!

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

Successfully merging this pull request may close these issues.

Exception in LatLngBounds.intersects() with chunkedLoading
2 participants