Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "provide" information to composer.json #330

Closed
wants to merge 1 commit into from

Conversation

mxr576
Copy link

@mxr576 mxr576 commented Jun 12, 2019

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:

  • This approach only works, if the asset-packagist repo is registered in the Drupal installation's root composer.json. The module can not register the asset-packagist repository because it has to be registered in the root composer.json. If the repository has not been registered, the installation fails with some mystical dependency error: The requested package bower-asset/swagger-ui could not be found in any version, there may be a typo in the package name. If someone did not read our module's install steps that detail the asset-packagist repo has to be registered first (developers always read documentation ;P) then the developer stuck with the installation process and probably s/he will send a new bug report to our GH issue queue.
  • If we add bower-asset/swagger-ui to our module's requirement, it could also happen that someone installs another module that requires npm-asset/swagger-ui. What happens? Best case, the same JS library gets installed to the same place twice, the later dependency overrides the files installed by the earlier one. Worst case, dependency hell.
  • It could also happen that someone already installed the Swagger UI directly from its Github repo with the VCS provider. Should the module install another version from the same library from a different source? Well, it should not.

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
Copy link
Author

mxr576 commented Jun 12, 2019

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

@rodrigoaguilera
Copy link

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.
hiqdev/asset-packagist#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
Copy link

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
Copy link
Member

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

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

@rodrigoaguilera
Copy link

@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
Copy link
Author

mxr576 commented Jun 17, 2019

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
Copy link
Author

mxr576 commented Jun 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants