Closed (won't fix)
Project:
Leaflet
Version:
3.0.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
20 Dec 2021 at 17:34 UTC
Updated:
26 Jan 2023 at 17:47 UTC
Jump to comment: Most recent
Comments
Comment #3
itamair commentedThis 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.
Comment #4
itamair commented@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.
Comment #5
itamair commentedWell 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 ...
Comment #6
itamair commentedLast 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 ...
Comment #9
hswong3i commentedComment #13
itamair commentedMarking this as RTBC as the 3.0.x branch is now working (somehow) with Leaflet libraries required via composer ...
Comment #14
hswong3i commented@itamair I give a rebase for all child issues ;-)
Comment #16
anybody@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 :)
Comment #17
anybodyComment #18
itamair commentedGood 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 ... ).
Comment #19
itamair commented@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)
Comment #20
anybody@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 :)