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

Load event may get lost #24

Open
letmaik opened this issue Aug 20, 2015 · 3 comments
Open

Load event may get lost #24

letmaik opened this issue Aug 20, 2015 · 3 comments

Comments

@letmaik
Copy link

letmaik commented Aug 20, 2015

The example code of the readme is:

new L.GPX(gpx, {async: true}).on('loaded', function(e) {
  map.fitBounds(e.target.getBounds());
}).addTo(map);

To me this looks like a race condition. If the constructor of L.GPX can load its data fast enough then the "loaded" event may get fired before the event handler is attached via on(..). I think you somehow try to work around that for the case when the XML is directly handed over (which would make it a blocking operation) by using setTimeout(..) without a delay value. But still, this doesn't solve the fundamental problem.

A possible way to solve this problem is to delay the loading until the object gets added to the map. This can be done by overloading the onAdd(map) method of the class and doing the loading there, essentially calling this._parse(gpx, options, this.options.async), and probably inside _parse calling the onAdd method of the parent class when done loading.

@mpetazzoni
Copy link
Owner

Very good points. Overloading onAdd(map) would definitely solve the issue, but in terms of "API" for L.GPX I like the fact that you can "prepare" your layer by creating it (var gpx = L.GPX(data);), and eventually use some of the methods it provides, before adding it to a map instance of your choice. It feels weird to be that parsing/loading the data is somehow tied to adding the layer to the map.

I guess another solution is to overload addTo(map) but for different reasons: when adding to the map, we could check if the data has been loaded already and fire the loaded event manually. This would cause the event to potentially be fired twice in most cases though.

@letmaik
Copy link
Author

letmaik commented Nov 2, 2015

Maybe we should look at the behaviour of some existing layer classes within leaflet which also do asynchronous loading. I can think of the tile layers only at the moment, and these definitely only start loading once you add them to a map.

@jkufner
Copy link

jkufner commented Apr 3, 2017

Don't forget that JS is single threaded. On Load event cannot be processed until your code returns to event loop. Therefore the code is completely fine.

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

3 participants