search

fxpio / composer-asset-plugin

NPM/Bower Dependency Manager for Composer
MIT License
893 stars 156 forks source link

Add "provide" information to composer.json #330

Closed mxr576 closed 3 years ago

mxr576 commented 5 years ago

Hi, first, thanks for this excellent library. Without this, I guess https://asset-packagist.org/ would not exist. :) 90% of the time I am working with Drupal, I am not sure whether you have heard about Drupal before. There is a problem that my colleagues and I are trying to solve, and I guess we are not the only one who is facing this problem day by day in the Drupal community. I also think we are not the only one with this problem in the PHP world, other frameworks, like Symfony, Yii could have the same problem.

Intro

In Drupal, we have smaller building blocks (libraries) called "modules" and "themes". A module could provide additional functionalities if it is enabled. A theme could change the look and feel of a site. Since Drupal 8, Drupal uses Composer to manage dependencies. Every Drupal module and theme could define its dependencies in its composer.json and Composer resolves them when a module or theme get installed. Drupal also has its "own Packagist" for Drupal components. A Drupal module/theme could have external Javascript dependencies that should be installed by Composer. At this moment, the majority of the Drupal community has a preferred way to install JS dependencies. They register the asset-packagist in a Drupal installation's root composer.json and pulls dependencies from there although there are other possible options to install a JS dependency, like using the VCS or the path repository providers, etc.

Problem

We have a module that uses the Swagger UI library to render API specification in Drupal: https://github.com/Pronovix/swagger_ui_formatter. This module does not work if the library is not installed. We added sanity checks that validate in Drupal whether the library is available although it would be great if we could add the Swagger UI as an actual (Composer) dependency to the module.

Currently, we only have the following option: Add npm-asset/swagger-ui or bower-asset/swagger-ui as a dependency to our module. This could ensure when the module gets installed, the Swagger UI library also gets installed with it, but this solution could cause other problems:

Proposed solution

If composer.json-s generated by asset-packagist would contain information about virtual packages that a certain JS package is compatible with, we could depend on the virtual package in Drupal modules/themes and we would not need to care about how and where a JS dependency gets installed.

This PR adds the following information to the bower-asset/swagger-ui's composer.json:

            "provide": {
                "swagger-ui/swagger-ui-implementation": "v3.22.3"
            },

With this information in place, we could add:

  "require": {
      "swagger-ui/swagger-ui-implementation": "^3.2",
      "symfony/dom-crawler": "~3.4|~4.0"
  }
mxr576 commented 5 years ago

Maybe asset/{name}-implementation would look better but naming collision could be a possible side effect of this vendor name prefix.

rodrigoaguilera commented 5 years ago

From what I understand this solution doesn't deal with problems 1 and 3 from the bullet list.

Since this plugin is replaced by foxy https://github.com/fxpio/foxy

And bower is on its way out since 2017 https://bower.io/blog/2017/how-to-migrate-away-from-bower/

I think the best solution for problem 2 is to remove bower altogether from this plugin and asset packagist. It will also give new room for solutions on mapping npm scoped packages. by having packages names like org-scope/package-name. https://github.com/hiqdev/asset-packagist/issues/97

As for Drupal I can't envision a proper solution to the third party asset management that doesn't involve adding a new packagist repo for npm assets on the composer template or adding assets to the Drupal packagist endpoint mirroring those on npm (like asset packagist). I think the latest is very unlikely since it involves The Drupal association.

In general I feel people don't care about managing their frontend libraries with composer because their frontend people already uses other tools on the theme like webpack but I feel this problems needs to be resolved for contrib modules that rely on third party libraries.

I'm at Drupal Dev Days Cluj, I will try to find you by asking on the pronovix desk and discuss further :)

ryanaslett commented 5 years ago

One of the primary issues is that composer was designed to be only a php dependency resolution tool, and in was never intended to be a generalized build tool that accounts for things like additional, non-php dependencies and assets that would be required for web frameworks.

The requirements around how that should be managed have never really been fully articulated. Are we talking about frontend libraries or not, and are we considering just 'add this extra library - download this file from this url and place it in this spot in the filesystem' OR do we also need something robust like a true dependency calculation ("swagger-ui/swagger-ui-implementation": "^3.2",).

Once we know what all the needs and wants are (and there are many, they've been piling up for a few years now), then we (The Drupal Association) can work on implementing a holistic solution that takes these kinds of assets into account.

francoispluchino commented 5 years ago

@mxr576 @ryanaslett You can read my comment in the issue drupal-composer/drupal-project#278.

@rodrigoaguilera You were faster than me to answer :-)

rodrigoaguilera commented 5 years ago

@ryanaslett Thank you for you comment here. It's great to know that someone from the Drupal association is aware of this issues :)

I feel the focus should be about having better dependency management at least for the libraries at core/assets/vendor/ in Drupal so they can be removed from the repo just like /vendor.

I don't want to pollute this project anymore. Where do you think is better to discuss this issues?

mxr576 commented 5 years ago

Thanks for the quick reactions!

@rodrigoaguilera

From what I understand this solution doesn't deal with problems 1 and 3 from the bullet list.

The "swagger-ui/swagger-ui-implementation" deals with 1-3 in a way that it adds Swagger UI as an actual dependency to our Drupal module but it does not specify any expectation about how and from where the Swagger UI gets installed. It could be added from its original GH repo with GIT subtree and registered as a package or installed from npm/bower-asset. What matters that Composer must know that the installed component provides a swagger-ui/swagger-ui-implementation. If we would add npm-asset/swagger-ui to a module we would say that only the npm-asset version is acceptable from Swagger UI, it can not be installed elsewhere.

I'm at Drupal Dev Days Cluj, I will try to find you by asking on the pronovix desk and discuss further :) Unfortunately I have not been at Cluj, I am rather delt with problems like this in Hungary :S :D But I hope you said hello to my colleagues and took apart in our climate hackathon :)

@ryanaslett

composer was designed to be only a php dependency resolution tool, and in was never intended to be a generalized build tool that accounts for things like additional, non-php dependencies and assets that would be required for web frameworks.

Yes, you are right and this was the right decision from the Composer creators.

we considering just 'add this extra library - download this file from this url and place it in this spot in the filesystem'

This is much like what the Libraries module did in Drupal 7 and it tries to do in Drupal 8, but there is no stable yet. Although, this module and this approach still do not solve the problem from the dependency management point of view. The Libraries module, just like our hook_requirements() implementation in the Swagger UI field formatter module can only notify (admin) users in runtime that there is a missing library. Although, ideally, when a module or a theme is installed, all its required dependencies (including assets) should be either installed with it or they should be already available in the system. Why? Because simply documenting a requirement and trust in developers that they always read the documentation is not enough - or they always test their PRs after they got merged to a QA env to find missing library notifications from Drupal in runtime.

This PR only tries to introduce a pattern for common virtual package name for assets tries to solve the dependency problem. These virtual packages could be used to ensure someone/something has already installed an asset dependency before a Drupal module/theme gets installed. (Yes, it does not solve that currently there is no defacto standard in Drupal 8 for asset locations.)

@rodrigoaguilera I understand this whole situation is not a problem of this library, but if this library and with that the asset-packagist.org repository does not start shipping these virtual package definitions we won't be able to build a standard around this. This functionality could be shipped by https://packages.drupal.org, but I do agree that it already does too much work and adding assets to that repo would just increase the problem in that end.

I feel the focus should be about having better dependency management at least for the libraries at core/assets/vendor/ in Drupal so they can be removed from the repo just like /vendor.

I completely agree with this but this would not solve the problem that this PR tries to solve.

Sorry for polluting this project with problems of the Drupal community. This PR probably never be merged, but these conversations around it could help to solve this problem somewhere/somehow in a closer future :)

mxr576 commented 5 years ago

I feel the focus should be about having better dependency management at least for the libraries at core/assets/vendor/ in Drupal so they can be removed from the repo just like /vendor.

Btw, as it used to be in Drupal ;-), "there is a module for that": https://www.drupal.org/project/vendor_stream_wrapper