Big Pipe changes causes multiple map attachments

Created on 30 January 2023, almost 2 years ago
Updated 1 February 2023, almost 2 years ago

Problem/Motivation

🐛 The attachBehaviors() for document is only called after Big Pipe chunks are processed Fixed (9.5) fixed a bug with Big Pipe that meant that Drupal.attachBehaviours() only ran after Big Pipe finished, which delayed attachments. Now it runs before and after.

This means that Drupal.behaviors.leaflet.attach now runs twice. This means the following:

  • add_features() gets called twice with the same data
  • $(document).trigger('leafletMapInit', [data.map, data.lMap, mapid]); gets called twice
  • $(document).trigger('leaflet.map', [data.map, data.lMap, mapid]); gets called twice

Steps to reproduce

Add some code that listens to leafletMapInit, it will be called twice, which isn't what you would expect for an init.

Proposed resolution

Use once() to prevent double runs - see https://www.drupal.org/docs/drupal-apis/javascript-api/javascript-api-ov... .

🐛 Bug report
Status

Fixed

Version

10.0

Component

Code

Created by

🇬🇧United Kingdom andrewbelcher

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @andrewbelcher
  • Status changed to Needs review almost 2 years ago
  • 🇮🇹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
  • 🇮🇹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 the map_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 the if (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 ...

    • itamair committed a3caa11c on 10.0.x
      Issue #3337537 by itamair, JeremySkinner, andrewbelcher: Big Pipe...
  • Status changed to Fixed almost 2 years ago
  • 🇬🇧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.

Production build 0.71.5 2024