Problem/Motivation

The current 1.0-beta versions of the Leaflet module for Drupal embeds the 3rd party Leaflet Js library dependency (v. 1.0.3) within the module's own code base and doesn't require further actions to the user.

However, third-party javascript libraries like Leaflet should be stored in the /libraries folder and ideally managed by composer.

Proposed resolution

Implement a composer-based dependency management that leverages the composer/installers package to download the library into Drupal's standard /libraries folder, as recommended in the official documentation for using composer to manage Drupal site dependencies. The patch here is primarily a documentation change, outlining the steps in the README to modify your website's main composer.json file. There is also one small code change to update the location of the javascript and css file dependencies out of this module and into the /libraries folder.

The approach in #11 requires adding leaflet to the custom "repositories" section, which works only from the main composer.json, not from the composer.json file included within this Drupal module.

The subsequent approach in #16 requires configuring Asset Packagist in your project's main composer.json file before requiring the module with Composer. Asset Packagist is emerging as a standard best practice in the community.

Backstory & references

* Official documentation for using composer to manage Drupal site dependencies.
* #2873160: Implement core management of 3rd-party FE libraries
* Previous discussion around managing the Leaflet library dependency in composer.json can be found on #2774237: Manage Leaflet library dependency in composer.json.

Issue fork leaflet-2942651

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

itamair created an issue. See original summary.

itamair’s picture

itamair’s picture

acbramley’s picture

Title: 8.x-1.0-beta2: Patch to Manage Leaflet library dependency in composer.json » Patch to Manage Leaflet library dependency in composer.json
Issue tags: -Composer, -library, -leaflet

I've recently used http://asset-packagist.org/ for a project to pull in the dropzonejs library with some good success. You need some specific config in your root JSON file though but it is pretty easy to set up.

The Thunder project uses this as well https://github.com/BurdaMagazinOrg/thunder-project/pull/5/files and #2843288: Make use of http://asset-packagist.org/ for fetching libraries

Removing redundant issue tags and version prefix on the title.

jwilson3’s picture

@itamair lots of trailing whitespaces in your patch (visible when reviewing with Dreditor).

itamair’s picture

The three back-ticks lines fence blocks of code in the Markdown syntax.
Better solutions and hints on reproducing the same markdown style but with other same efficient syntax are really welcome.

jwilson3’s picture

If you're looking to replace the backticks you can just indent all code lines 4 spaces. If the code is supposed to be a sub-element of a list element, then you need to indent 8 spaces. To simplify all this, you could convert your numbered lists to headings instead, kind of like what we've done here:

https://github.com/BluesparkLabs/gulp-frontend-tasks/blob/master/README.md

But, to clarify, I'm not talking about the backticks specifically when I refer to the trailing whitespace.

jwilson3’s picture

+++ b/README.md
@@ -18,8 +18,52 @@ It is done simply running the following command from your project package root
+2. __Require/Add the Leaflet JS library via Composer:__  ¶
+In order to do this, it is requested before to add the proper ¶
+   Leaflet JS library repository to your main composer.json file. ¶
+   It will be sited into the /Library folder. ¶
+   ¶
...
+   ¶
...
+  * Verify that the proper library destination folder is ¶
+  defined under "installer-paths" index in you (main/root) composer.json, ¶
...
+  From your project package root (at the main composer.json level) run:  ¶
+  __$ composer require leaflet/leaflet:~1.0__  ¶
...
-2. Enable the module to be able to use the configurable __Leaflet Map as 
+3. Enable the module to be able to use the configurable __Leaflet Map as ¶

These are all the lines with trailing whitespace.

itamair’s picture

tnx @jwilson3!
Don't get why my IDE (phpstorm) is not trimming trailing whitespaces

[*.md]
trim_trailing_whitespace = true

I hope this new one looks better.

jwilson3’s picture

Version: 8.x-1.0-beta2 » 8.x-1.x-dev
Assigned: itamair » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
2.69 KB

Patch in #9 no longer applies. Here is a re-roll with some grammar and markdown formatting fixes for the README.

jwilson3’s picture

jwilson3’s picture

jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

Status: Reviewed & tested by the community » Needs work

Hrm. After reading the core issue #2873160, it looks like some best practices are emerging around using Asset Packagist.

See Comment #41 from sun here, with examples of how this is being done in another module #2873160-41: Implement core management of 3rd-party FE libraries

+  "repositories": [
+    {
+      "type": "composer",
+      "url": "https://asset-packagist.org"
+    }
+  ],
+  "require": {
+    "bower-asset/leaflet": "^1.0.3"
+  }

And

/**
  * Implements hook_libraries_info().
 */
function photoswipe_libraries_info() {
  $libraries['photoswipe'] = array(
    'name' => 'Photoswipe',
    'vendor url' => 'https://github.com/dimsemenov/PhotoSwipe',
    'download url' => 'https://github.com/dimsemenov/PhotoSwipe/archive/master.zip',
    'version arguments' => array(
      'file' => 'dist/photoswipe.min.js',
      'pattern' => '/v([\d.]+)/', // PhotoSwipe - v4.1.1 - 2015-12-24
      'lines' => 1,
      'cols' => 30,
    ),
  );
  return $libraries;
}

Maybe we should give that a try here?

itamair’s picture

thanks @jwilson3 ... !
Yes sure. Make all your investigation and further tests and please share here all your achievements.
Any solid improvement and amended patch upon this task is really welcome!

jwilson3’s picture

Title: Patch to Manage Leaflet library dependency in composer.json » Manage Leaflet library dependency with Composer & Asset Packagist
Issue summary: View changes
Status: Needs work » Needs review
FileSize
5.85 KB

Here is a patch that adopts the Asset Packagist methodology for managing the 3rd party Leaflet js package, which is emerging as a new standard best practice in the community.

The required code changes are minimal and mostly this is an overhaul of the README. I didn't bother doing an interdiff this time.

I looked into documenting an installation method without Composer and thereby adding the hook_libraries_info block mentioned in the previous comment, but this turned out to be way too complicated because the Leaflet Drupal module depends on Geofield, which itself uses composer to install the required GeoPHP 3rd party vendor library. I would say this module is now tied to Composer for installation — not necessarily a bad thing!

jwilson3’s picture

Status: Needs review » Needs work

@itamair Before you decide to commit this, I've taken some of the instructions from this patch in the README.md and copied them and cleaned them up in the official Drupal.org documentation here:

https://www.drupal.org/docs/develop/using-composer/using-composer-to-man...

And here:

https://www.drupal.org/docs/develop/using-composer/managing-dependencies...

I also mentioned this change on #2873160-50: Implement core management of 3rd-party FE libraries.

Assuming this change doesn't create some push-back or get removed by someone, we could probably trim down the README to just reference that documentation before committing the change.

Cheers.

itamair’s picture

thanks @jwilson3 ... will review this asap, following your instructions, and will come back accordingly

itamair’s picture

Status: Needs work » Needs review
itamair’s picture

hi @jwilson3 ... and thanks so much for all this you did.
I didn't test it but I am confident that it would work properly.

Yes ... I agree that we might reference the official Drupal 8 & Composer specific documentation on third party libraries,
but without trimming that much the Leaflet (with Composer) one. Extended documentation is better (for normal users) than the too synthetic one.

Some further question, if we agree on the commit of this Composer version, then:
- the actually embedded leaflet library (/js/leaflet) should be removed too, by the patch, right?
- I wouldn't commit this into the actual 8.x-1.x branch ... due to the big change that would introduce, and also the remove of the Leaflet library.
Do you agree that this might be worth a brand new 8.x-2.x of the Leaflet module?

Or how would you move on / commit, instead, on all this?

vierlex’s picture

Would it be too much overhead to leave the current embedded leaflet library,
but introduce a new one which uses the new packagist workflow?

So there is still a fallback and there is time to migrate.
You could throw a notice at the Statuspage or whereever to signal Users that there is a new workflow in town,
and currently, the internal leaflet.js is used, but shouldnt and in the future will be deprecated, like 8.x-2.x

itamair’s picture

2 possibile leaflet libraries? M
Mmmhhh
would mean also a leaflet configuration page from where the user should be able to choose which position to use,
and the application logic should be extended to behave and work consequently: load from the module embedded one, or from the library folder one, depending on the leaflet configuration settings.

This would mean and need some major work ... (that actually I don't have time to bring on).
Eventually it would be nice to have (besides the nice wishes) also a nicely written and tested patch (php & drupal 8 standards compliant) that makes all this ... also embedding the #16, properly re-rolled to apply to the actual dev branch.

itamair’s picture

Status: Needs review » Needs work
tobiasb’s picture

Version: 8.x-1.x-dev » 2.1.x-dev
Status: Needs work » Needs review
Parent issue: #2774237: Manage Leaflet library dependency in composer.json »
Related issues: +#2774237: Manage Leaflet library dependency in composer.json
FileSize
36.66 KB

All libraries are gpl-compatible therefore we could add them to code like drupal core does it.

But I also do not think cdn or asset-packagist is a good way.

This patch use the hook_library_info_alter to allow the user, to managed the dependencies via composer.

Btw. with a non-cdn-version of geoman I do not have a problem with #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS.

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

hswong3i’s picture

Category: Plan » Feature request

A totally rewrite version:

  • Since leaflet_markercluster submodule just handle the JS library initialization, I give it a merge to its parent leaflet.libraries.yml
  • Remove all locally vendored JS libraries, in order to avoid any Drupal.org related licensing issue
  • Update JS libraries search path into /libraries/*, so assuming install by using composer + https://asset-packagist.org
  • Update README.md for how to install corresponding JS libraries

hswong3i’s picture

hswong3i’s picture

https://git.drupalcode.org/project/leaflet/-/merge_requests/5 contain all of my recent patches for leaflet all-together, with screenshot as below:

https://git.drupalcode.org/project/leaflet/-/merge_requests/5

itamair’s picture

Thanks so much @hswong3i for your contribution. It is a lot of it ...
Not sure if it is better to review all in one shot here, or go through all the issues you provided and review one by one.
Definitely I will need some time to review all this ... and I will try to find asap.

hswong3i’s picture

@itamair to keep it simple please kindly review https://git.drupalcode.org/project/leaflet/-/merge_requests/7, which is the foundation for all follow up MR. Most likely about remove embed libraries and update in documentation.

Benefit for installing libraries with composer: https://www.drupal.org/project/leaflet/issues/2942651#comment-14307721

Once this land we could review other MR together in detail ;-)

  • hswong3i committed bac90b5 on 2.1.x
    Issue #2942651 by jwilson3, itamair, hswong3i: Manage Leaflet library...

  • hswong3i committed bac90b5 on issue/leaflet-2942651-2942651-manage-leaflet-library
    Issue #2942651 by jwilson3, itamair, hswong3i: Manage Leaflet library...

  • itamair committed eae259f on 2.1.x
    Revert "Issue #2942651 by jwilson3, itamair, hswong3i: Manage Leaflet...
itamair’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#3255306: Manage Leaflet libraries dependencies with Composer & Asset Packagist #2

@hswong3i too(ooooooo) much confusion here ...
Sorry if I think that a Drupal post (previously existing) cannot be filled (polluted) with all these posts/updates, also with mixed MRs ... it becomes basically unreadable/un-understandable for however (and mostly for not super skilled coders).

Let's reorder this:

1. First of all imho the https://git.drupalcode.org/project/leaflet/-/merge_requests/5 is out of context here, as it is addressing/wrapping other tasks not really related with Composer & Asset Packagist libraries dependencies, so please create a separate Feature Request issue with all that,
and also (please) refer in the Issues all the singles Leaflet issues that you opened singularly ...

2. Trying to test your previous https://git.drupalcode.org/project/leaflet/-/merge_requests/7 I had to Merge into the 2.1.x ... so it is gone..
But I recreated a new MR copy of it into a new "Manage Leaflet libraries dependencies with Composer & Asset Packagist #2" leaflet feature request ...
So let's continue all this over there