Hello,

We install all our JS libraries with Asset Packagist. This makes updating them much easier.

So this is basically how we installed toc.js:

{
	"repositories": {
		"type": "composer",
		"url": "https://asset-packagist.org"
	},
	"require": {
		"oomphinc/composer-installers-extender": "^1.1",
		"npm-asset/firstandthird--toc": "^0.3.2"
	},
	"extra": {
		"installer-types": ["npm-asset"],
		"installer-paths": {
			"web/libraries/toc": [
				"npm-asset/firstandthird--toc"
			],
			"web/libraries/{$name}": [
				"type:drupal-library",
				"type:npm-asset"
			]
		}
	}
}

This means however that the library is not where toc_js expects it. So we had to apply the attached patch.

But I guess a more flexible solution would to have a way to set the path in the module config?

CommentFileSizeAuthor
#9 interdiff.txt2.17 KBdom.
#9 allow_npm_dependencies_install--3089718-9.patch5.25 KBdom.
#7 allow_npm_dependencies_install--3089718-7.patch3.91 KBdom.
toc.diff820 bytesprudloff

Comments

prudloff created an issue. See original summary.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

It's not a common case to set a path library in a module's configuration.

prudloff’s picture

prudloff commented

Could an alternative solution be to use a composer.libraries.json file like Webform does (https://www.drupal.org/docs/8/modules/webform/webform-libraries)?
This way, people using composer-merge-plugin don't have to worry about downloading the library manually.

pcate’s picture

pcate commented

One current workaround for this would be to use hook_library_info_alter to change the js file path.

You could also use libraries-override in a theme .info file to make this change.

dom.’s picture

dom. commented

+1 for composer.libraries.json solution suggested at #3, it is becoming a more and more seen pattern and I use it currently in my modules.

Also those are core issues to watch regarding how to handle this:
- #2873160: Implement core management of 3rd-party FE libraries
- #2605130: Best practices for handling external libraries in Drupal 8/9/10 and 11

Final note: I suppose this would also be needed to have an up-to-date library:
#3093289: Update to use latest version of TOC.js library.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Version: 8.x-1.6 » 2.0.x-dev

I suppose this would also be needed to have an up-to-date library

Yes.

Edit: note, upgrading the library means certainly some works to refactor existing code of this module, and perhaps remove some parts of the plugin methods which have been override.

dom.’s picture

dom. commented
Status: Active » Needs review
StatusFileSize
new allow_npm_dependencies_install--3089718-7.patch3.91 KB

This issue is tricky actually because :

  1. current module version does not allow file to be found in /dist folder
  2. because of (1), it is not compatible with npm approach
  3. moving the necessary file to /dist would require a module upgrade version (as opposed to minor) because it would not be backward compatible and would require a manual change in librarie folder for most.

The approach I suggest here is :
> add a composer.libraries.json to allow people to use composer only install. Therefore, using the wikimedia/composer-merge-plugin plugin, you could use in your composer.json : 

"extra": {
        "merge-plugin": {
            "include": [
                "modules/contrib/toc_js/composer.libraries.json"
            ]
        },
}

either do it before installing the module, or after and run composer update --lock to download the library
This is the approach taken by webform and many more modules following it's example.
> leave the library definition in toc_js.libraries.yml as is to maintain retro-compatibility
> add overrides to have it able to detect the dist folder
> add a @todo to remove it when upgrading to new major version with #3093289: Update to use latest version of TOC.js library.

The patch attached for review does exactly this.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Hi Dom. and thanks.

Some reviews. 

  1. +++ b/composer.libraries.json
@@ -0,0 +1,42 @@
+    "issues": "https://www.drupal.org/project/issues/toc_js?version=8.x",

    Use the latest default branch 2.0.x 

  2. +++ b/composer.libraries.json
@@ -0,0 +1,42 @@
+  "repositories": {
+    "masonry": {

    certainly the name has to be changed here 

  3. +++ b/toc_js.module
@@ -401,3 +401,27 @@ function toc_js_form_node_type_form_builder($entity_type, NodeTypeInterface $typ
\ No newline at end of file

    missing new line

Also we need to update the README (and after the project page, once done).

Is the "right" (currently supported) version 0.3.2 is still downloaded with your patch ?

dom.’s picture

dom. commented
StatusFileSize
new allow_npm_dependencies_install--3089718-9.patch5.25 KB
new interdiff.txt2.17 KB

1. Let's just remove the version anyway
2. Guess where I took the composer.libraries.json file from ^^
3. Added
4. Updated the README to add the new available download methods.

New patch and interdiff attached.

Yes, the composer.libraries.json point the version 0.3.2. The path is https://github.com/jgallen23/toc/archive/greenkeeper/update-all.zip which is the zip download link on github for that version (given also in the module page itself).

Thanks a lot for the review.

  • Dom. authored 14af54a on 2.0.x 
    Issue #3089718 by Dom., prudloff, flocondetoile: Set toc.js path
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Needs review » Fixed

Committed. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.