Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork leaflet-3255306

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

itamair created an issue. See original summary.

itamair’s picture

Version: 2.1.x-dev » 3.0.x-dev
Status: Active » Needs work
Parent issue: » #2942651: Manage Leaflet library dependency with Composer & Asset Packagist

This Feature Request is the continuation of this Parent one: https://www.drupal.org/project/leaflet/issues/2942651#comment-14345058

This issue MR is a copy (and continuation of this original one: https://git.drupalcode.org/project/leaflet/-/merge_requests/7)
and a dedicated 3.0.x-dev branch of the Leaflet module has been created to test this implementation.

itamair’s picture

@hswong3i I don't know if I am missing something, but I couldn't find from my review.

In my testing of this: https://git.drupalcode.org/project/leaflet/-/merge_requests/13
I should report the following:

1. the Leaflet module is expecting to find both the "leaflet" and the "leaflet.markercluster" libraries in the /libraries ... but they are not, after Composer require/update (and rather they looks located in the vendor/npm-asset folder (along with the other leaflet libraries, such as: leaflet-fullscreen, leaflet-gesture-handling, etc.)
so nothing is working and everything breaks when switching an existing Leaflet Drupal project on 8.x & 2.x branches into this 3.0.x one;

2. if point 1.x worked correctly how do you think a seamless upgrade path should/could be implemented for existing 8.x & 2.x branches based ones? I cannot see any easy one. Thousands of Leaflet based Drupal instances should be able to smoothly upgrade.

So probably the only possibile solution for all this should be to spin off a new supported Leaflet 3.0.x branch/release ...
but the idea of keeping 2 parallel projects, updating and aligning their respective features is absolutely not in my plans and possibilities, and currently I am the only maintainer of the Leaflet module for Drupal 8 and 9.

itamair’s picture

Well no. I am making too much drama on this, may be.
We would just need to add clear instructions on how to upgrade from 2.1.x into 3.0x versions ...

itamair’s picture

Status: Needs work » Needs review

Last 3 commits correct all the issue that I found in the original implementation (https://git.drupalcode.org/project/leaflet/-/merge_requests/7).

1. npm-assets should be installed into "web/libraries/{$name}" (and not: "/libraries/{$name}");
2. at least the leaflet_markercluster submodule info.yml file should be preserved, for Backword compatibility, as removing the leaflet_markercluster code base could break existing Leaflet 8 & 9 instances;

With these corrections my existing Leaflet Drupal 8 & 9 playgrounds still work fine ...

hswong3i made their first commit to this issue’s fork.

itamair’s picture

Status: Needs review » Reviewed & tested by the community

Marking this as RTBC as the 3.0.x branch is now working (somehow) with Leaflet libraries required via composer ...

hswong3i’s picture

@itamair I give a rebase for all child issues ;-)

anybody’s picture

@itamair do you know what's the status here? is MR!15 still correct and required? It looks like it contains a lot of unrelated changes?

I thought, the approach here was to provide the used libraries locally instead of CDN (optionally)?

The background of my question is that on a page using leaflet I found CDN code loading from jsdelivr and unpkg and I'm currently trying to implement a solution for COOKiES module to block these. That needs a solid foundation :)

anybody’s picture

itamair’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

Good point @Anybody ...
Please note that this issue is applying to the 3.0.x-dev branch, that is only experimental, not supported now and (personally) not going to be supported in the close future, not to break Backward Compatibility and because it doesn't offer an easy upgrade to existing users.

Also worth to mention that this issue goal is not to use CDNs but basically move the Leaflet module dependency libraries in the Drupal app "libraries" folder, and manage them with Composer & Asset Packagist ... that in case of the Leaflet module is not a great and strictly needed improvement (imho) for the following reasons:

1. every other module (in the Drupal app) needing any Leaflet related library is (very very probable) going to depend from the Leaflet module itself (and if it is not the case it won't use/require the Leaflet module at all);
2. hook_library_info_alter is always available to alter/customise the Leaflet libraries definitions ...

I am actually working on a new major upgrade of the Leaflet module (with new extended/cool functionalities and full backward compatibility with 2.x branch last release) and its Leaflet and Leaflet Plugins libraries are going to be (even more) embedded into it.
(I have tested that using CDNs ones cause issues in using the Leaflet widget with "inline entity form" and paragraphs widgets ... ).

itamair’s picture

@Anybody FYI I just deployed a new leaflet 2.2.12 release where leaflet.gesture_handling and leaflet.fullscreen libraries files have been moved inside the Leaflet module code base (now all Leaflet Js dependency libraries reside into the module)

anybody’s picture

@itamair, thanks a lot!

What we need is a way to provide the libraries locally and make leaflet GDPR compatible in general. I just commented in #3086698: Use Leaflet GDPR conform and think a combination with COOKiES might be a good way.

So if the site owner is able to replace all libraries by local ones (for the future, best might be to solve it like Webform?), that's great. But let's proceed in that issue, we shouldn't mess up this one further :)