Islandora / documentation

Contains islandora's documentation and main issue queue.
MIT License
104 stars 71 forks source link

Install JS libs with composer #1168

Open dannylamb opened 5 years ago

dannylamb commented 5 years ago

Add assets-packagist.org to main composer.json repositories for external javascript libraries. Then we can install external js using composer. :champagne: :rocket:

dannylamb commented 5 years ago

Thx for pointing this out @nikathone

ibrahimab commented 5 years ago

Isn't there already a way to install js libraries like npm en yarn? What is the reason for using composer?

bryjbrown commented 5 years ago

@ibrahimab I'd say the biggest reason for using Composer is that we are already using it to install everything else. It makes more sense to have our primary tool do more than to add additional tools.

whikloj commented 5 years ago

@ibrahimab Drupal 8 uses composer to install modules, so this allows us to define external JS libraries and have them installed along with the PHP module. This is not intended to install only JS libraries.

ibrahimab commented 5 years ago

I get that, but I don't think this is the way to go. There is a reason npm/yarn exists. They do a lot more than just fetching code and saving to disk. They do a lot of optimization like walking the tree to just include the code that the application uses.

I have looked at assets-packagist and it just allows you to download assets into the vendor folder. This also deviates from standards (requiring code from the vendor folder instead of node_modules).

Besides, Drupal 8 already has a way of using javascript in modules: documentation

This would create extra work, with no definitive benefits and deviates from standards.

dannylamb commented 5 years ago

Yeah, but we're not doing front end work here. This is just so that when people install our viewer modules with composer they don't have to manually deploy the library to the /libraries folder. That's it. If we were making a theme or doing a headless frontend I wouldn't use composer either.

ibrahimab commented 5 years ago

Why wouldn't you use the default drupal 8 method, defining "*.libraries.yml" file(s) then?

nikathone commented 5 years ago

Hi Ibrahim, defining *. libraries.yml doesn't download/deploy the files unless you use an external url. This approach of using composer have been adopted by widely known Drupal contribution like https://github.com/acquia/lightning-project/blob/6423cabdb4b5a300a34c8e48e141c83275bcb0ae/composer.json#L52, https://github.com/BurdaMagazinOrg/thunder-project/blob/7d543bca5dd4b202a3003e9da36e26e3fbef14cb/composer.json#L33 or https://github.com/goalgorilla/social_template/blob/b923b93033afe45540fcabba6589a624c985abe6/composer.json#L24. And there are many other modules which advice to use the same approach for downloading their external JavaScript assets.

ibrahimab commented 5 years ago

@nikathone Isn't OP talking about external projects? And other projects doing it too is not a good enough reason for me. If it was up to me, I would use npm, setup webpacker and use libraries.yml to include the compiled result.

nikathone commented 5 years ago

I think we are not compiling anything right? It's just a way to download a javascript dependency using composer. What is OP? by the way.

The reason I gave those projects as an example is that most of the people maintaining them are also drupal core maintainers. Another example is the slick module, in slick.libraries.yml you have an external library declared like

  remote: http://kenwheeler.github.io/slick/
  version: 1.x
  license:
    name: MIT
    url: https://github.com/kenwheeler/slick/blob/master/LICENSE
    gpl-compatible: true
  js:
    /libraries/slick/slick/slick.min.js: { weight: -3, minified: true }
  css:
    base:
      /libraries/slick/slick/slick.css: {}
  dependencies:
    - core/jquery

then you can run $ composer require bower-asset/slick-carousel:^1.8 to get slick assets downloaded under <project>/web/libraries.

This doesn't mean that the only way to get the assets locally is to use the composer approach though. In the slick drupal module readme, under requirements, they actually add an option to do it manually.

Slick library: o Download Slick archive >= 1.6 && <= 1.8.1 from https://github.com/kenwheeler/slick/releases o Master branch (1.9.0) is not supported. Instead download, rename one of the official slick releases to slick. Extract and rename it to "slick", so the assets are at: /libraries/slick/slick/slick.css /libraries/slick/slick/slick-theme.css (optional if a skin chosen) /libraries/slick/slick/slick.min.js

ibrahimab commented 5 years ago

What is OP? by the way.

Original Poster

This doesn't mean that the only way to get the assets locally is to use the composer approach though. In the slick drupal module readme, under requirements, they actually add an option to do it manually.

This is why I oppose getting assets through composer, I just think islandora should use standards (npm). Why would we deviate from how everyone else does it because "It makes more sense to have our primary tool do more than to add additional tools. (@bryjbrown)". Using the best tool for the job should be the rule (referring back to the optimizations npm does behind the scenes).

jordandukart commented 3 years ago

Is this still something that we want to pursue? I think this isn't a good approach to handling JS dependencies as the above states.

ibrahimab commented 3 years ago

I also forgot to mention, using npm will significantly reduce downloads, allow developers to override things without going into core modules (like sass variable overrides), css imports css natively, es6 module imports natively, bundling, packaging (even per page/module) and much more. Why not use it?

seth-shaw-unlv commented 3 years ago

I'm in favor of using composer for adding external libraries. I've recently started using it for my projects and it has made site setup a lot easier than attempting manual install, e.g.

"repositories": {
        "drupal": {
            "type": "composer",
            "url": "https://packages.drupal.org/8"
        },
        "asset-packagist": {
            "type": "composer",
            "url": "https://asset-packagist.org"
        }
    }

...

        "installer-paths": {
            "drush/Commands/{$name}": [
                "type:drupal-drush"
            ],
            "core": [
                "type:drupal-core"
            ],
            "modules/contrib/{$name}": [
                "type:drupal-module"
            ],
            "modules/custom/{$name}": [
                "type:drupal-custom-module"
            ],
            "profiles/contrib/{$name}": [
                "type:drupal-profile"
            ],
            "profiles/custom/{$name}": [
                "type:drupal-custom-profile"
            ],
            "themes/contrib/{$name}": [
                "type:drupal-theme"
            ],
            "themes/custom/{$name}": [
                "type:drupal-custom-theme"
            ],
            "libraries/{$name}": [
                "type:drupal-library",
                "type:bower-asset",
                "type:npm-asset"
            ]
        }

allowed me to simply composer require npm-asset/nouislider:^14.

alxp commented 3 years ago

I think this is a good idea, and is well-supported by Drupal.

It would mean we would need to have our own base composer project, since you can't declare repositories in modules' composer files. THis would be a good idea for a few reasons, including a current problem upgrading several moduels at once that I ran into here: https://github.com/Islandora-Devops/islandora-playbook/pull/202#issuecomment-938041356

Currently the ansible playbook manually creates the library directory structure and downloads individual libraries as tasks in the roles/internal/webserver role tasks.

These are the steps required for a single library:


# masonry library is required by content_browser and not installed by composer due to issue 2971165.
- name: Create drupal library directory.
  file:
    state: directory
    path: "{{ drupal_external_libraries_directory }}"
    owner: "{{ webserver_app_user }}"
    group: "{{ webserver_app_user }}"

- name: Unarchive masonry library.
  unarchive:
    src: "https://github.com/desandro/masonry/archive/v3.3.2.zip"
    dest: "{{ drupal_external_libraries_directory }}"
    creates: "{{ drupal_external_libraries_directory }}/masonry"
    remote_src: yes

- name: Rename masonry directory.
  command: mv "{{ drupal_external_libraries_directory }}/masonry-3.3.2" "{{ drupal_external_libraries_directory }}/masonry"
  args:
    creates: "{{ drupal_external_libraries_directory }}/masonry"

This also makes keeping track of what version of Masonry is installed difficult. If Composer could have visibility into libraries it would be easier on a site maintainer.

Moving to a different composer.json file I think would warrant its own issue, but I'd be in favour of it.

rosiel commented 3 years ago

Using a composer project would solve a number of problems currently faced by the community.

  1. The playbook doesn't only need to create a good installation from scratch, it needs to be able to be run to update existing installations. The way we specify requirements in Ansible goes one-at-a-time, which makes it very very difficult for us to change the version specs. As happened in https://github.com/Islandora-Devops/islandora-playbook/pull/202, doing so broke the playbook. Having whatever composer requirements we want to have in a different format (a composer.json and/or lock) would make updates possible.
  2. We recommend the playbook as a way to easily install Islandora to folks who are friendly with the command line but not necessarily developers. Currently, we composer require latest versions. If (not if, when) one of our dependencies screws up and makes a breaking change without implying it in their semantic versioning, then newcomers to islandora get frustrated that they can't install it. Having the playbook able to install by default a Drupal set of requirements that have been tested together and are known to work could be done by this method (a lock file or a composer.json that specifies exact versions). As @jordandukart pointed me to, this is the method used by Drupal core: https://github.com/drupal/core-recommended/tree/9.3.x. This method would allow this "known good state" to be updated by the community much more easily than the previous solutions to this use case - the now-deprecated islandora/8 vagrant base box, and the islandora-sandbox which is a dump of an entire drupal codebase.
  3. For non-throwaway cases (staging or production sites) that use the playbook, people will need to be able to manage their own Drupal composer requirements, yet still use the playbook to perform updates. As @alxp mentioned to me, you can do this by making the composer project to install an ansible variable, that will default to ours but can be overridden to point to a fork. Again, it seems this will make it simpler to separate changes that are part of drupal site management (i.e. which modules are installed on your own site) from those regarding infrastructure.
  4. Finally, Ansible and Docker use different syntaxes to include composer requirements, which means the Playbook and ISLE suffer from version drift. A project like this would (i believe) be able to be used by either so the deployment methods could give an equivalent "out of the box" experience.
dannylamb commented 3 years ago

We do this at Born Digital and it works really really well. :+1: from me.

adam-vessey commented 3 years ago

Not... so sure about this, in the general case? Seems like it would work for some kind of more-or-less turn-key project; however, having the use of asset-packagist be mandatory, baked in the composer.json of modules requiring libraries, when said modules can't actually add additional repos to the root project seems... less-than ideal? Could see adding them in the suggest section maybe, with some kind of description about adding asset-packagist to one's repos in their project's root composer.json and/or pointing at documentation about it, but ultimately leaving the installation of the library with the options:

seth-shaw-unlv commented 3 years ago

@adam-vessey, as @alxp noted, this would only be for the Isle and Playbook composer files where you really want something to be turn-key. This wouldn't stop institutions from removing the asset-packagist and using a CDN for their own deployments. Modules wouldn't add the asset-packagist packages to their individual composer files, unless they added it to "suggests" as you propose.

adam-vessey commented 3 years ago

@seth-shaw-unlv: I don't see any constraints on the scope in which this is to be applied in @alxp's comment (or any other comments in the issue)... unless it requires reading between the lines, or happening in comms out-of-band?... which is why I bring it up here, explicitly.

Given it's counter-intuitive nature, of the libraries being dependencies of the modules, but having to specify the repositories in the root/project composer.json in order to make reference to them, just wanting to make sure the we don't get into the situation of having to have in the documentation something like: "go add this repo to your composer.json before composer requireing this module", kind of thing.

If this is just to be in a Composer project that can be used optionally (and used between both the Playbook and ISLE and wherever desired), without strongly requiring asset-packagist in any of the different modules, then yeah, I'm fine with this.

seth-shaw-unlv commented 3 years ago

Sorry, that was me reading between the lines based on the "you can't declare repositories in modules' composer files" line and the way adding asset-packagist works. We would only be able to add them to the Isle/Playbook composer files.

I often see Drupal module descriptions which include notes about either using asset-packagist or installing the libraries via CDN/manually. We should be doing the same for the relevant modules.

jordandukart commented 3 years ago

If we're still using Drupal as a guiding force they are moving to package.json in core at least as of 9.3.x: https://www.drupal.org/node/3197544.