- Issue created by @andrewbelcher
- Status changed to Needs review
almost 2 years ago 2:03pm 30 January 2023 - 🇮🇹Italy itamair
thanks a lot @andrewbelcher.
Could you test/QA and review the attached patch that I built up following your suggestion, with the once() construct/pattern.
From my testing it indeed limiting each inside callback to just once in the page ...
I also extended to the Leaflet Widget implementation.Please let me know if it works as you would expect now, and the code looks appropriately solid. Feel free to add your corrections/amends ... if any.
- 🇬🇧United Kingdom JeremySkinner
hi @itamair I was going to review this on @andrewbelcher's behalf but we're unable to apply this patch to the 2.2.12 version of leaflet which we're using - it looks like the patch only supports 10.x.
I've attached a 2.2.x compatible version of the same patch. I've tested this against our application and it appears to be working correctly.
- Status changed to RTBC
almost 2 years ago 9:19pm 31 January 2023 - 🇮🇹Italy itamair
Well ... actually better inspecting and testing the above approach and solution I realised that is not a working general fix.
With the above solution the once() condition will be triggered only for the all document load ("html"), and that is not good.
It won't trigger the Leaflet Map initialisation in case of Leaflet Map inside Ajax requests and contexts
Typical case the Paragraphs Widgets, in case the Geofield is located as part of a Paragraph ...In the end, if the Big Pipe triggers twice the Drupal.behaviour ... well it could be right not to stop its second call.
What we need to stop is the re-initialisation of a Leaflet Map inside a container that already have an initialised one.
This assumption made me discovering a weakness in the existing Leaflet module code, that
checks if themap_container.data('leaflet') === undefined
here:
https://git.drupalcode.org/project/leaflet/-/blob/10.0.x/js/leaflet.drup...
that is right ...
and if not it (else) tries to re-add-features here:
https://git.drupalcode.org/project/leaflet/-/blob/10.0.x/js/leaflet.drup...
that is totally wrong ... and not needed (no real cases to cope with that).The new attached patch is thus resolving all this, with the following:
- applying itself to the 10.0.x branch (please note that the 2.2.x is not supported anymore, and everybody should update, relying in backward compatibility).
- removing the "else" statement (jon needed);
- wrapping all the subsequent code of the Drupal.behaviors.leaflet in theif (map_container.data('leaflet') === undefined) {
statement ...I have tested all this and works solidly, fixing this issue, hence I am going to commit this into 10.0.x branch and deploy a new Leaflet 10.0.8 release ...
@andrewbelcher, @JeremySkinner confident you will be easily able to translate that into 2.2.x, but keep in mind the 10.0.x branch is the way to go now ...
- Status changed to Fixed
almost 2 years ago 9:20pm 31 January 2023 - 🇬🇧United Kingdom JeremySkinner
Thanks @itamair, we'll work towards upgrading to 10.x so we can make use of this.
Automatically closed - issue fixed for 2 weeks with no activity.