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 asset-packagist repository #286

Closed
wants to merge 6 commits into from
Closed

Conversation

rodrigoaguilera
Copy link
Contributor

Be able to download download third party assets with commands like
composer require bower-asset/dropzone=~4.2

closes #278

@fidelix
Copy link

fidelix commented Aug 23, 2017

This works very well for me. I can recommend.

@rodrigoaguilera
Copy link
Contributor Author

Acquia recently released a blog post explaining this method.
https://lightning.acquia.com/blog/round-your-front-end-javascript-libraries-composer

I hope it becomes a standard workflow.

I will rebase the PR ASAP.

@FatherShawn
Copy link

I'm also using this method in my projects

@hawkeyetwolf
Copy link

Adding component into the installer-types and library path definition would also add support for all the packages in the components org on Packagist proper.

@hawkeyetwolf
Copy link

hawkeyetwolf commented Oct 27, 2017

This approach doesn't work for contrib modules right now because they can't safely require asset-packagist dependencies. Per the composer docs:

Repositories are only available to the root package and the repositories defined in your dependencies will not be loaded. Read the FAQ entry if you want to learn why.

And this causes two problems:

  1. Breaks testbot, since she doesn't have the asset-packagist repository definition in her root project (right now).
  2. Prevents requiring the module via Composer by any site which doesn't have the asset-packagist repository defined. We can put big bold words on the d.o project page and in the README, but it still seems problematic.

What do you think? Is there any way around this?

@sun
Copy link

sun commented Apr 6, 2018

@derekderaps As repositories are custom by design in Composer, this seems to be the only viable solution – at least for now.

For the time being, this PR resolves https://www.drupal.org/project/drupal/issues/2873160

@rodrigoaguilera Any chance you could rebase it to resolve the conflicts?

@weitzman
Copy link
Contributor

weitzman commented Apr 6, 2018

In addition to Lightning, Drupal Commerce now ships with this approach - https://www.drupal.org/project/commerce/issues/2916058. +1 from me

@heddn
Copy link

heddn commented Apr 6, 2018

I was originally a little reluctant to add this to composer. Reason for opposition: my theme needs certain css and fonts. Using bower (npm these days) it is fairly easy to require them. And then when anyone consumes my theme, they have everything packaged for them. In the theme. Just run yarn install && yarn run gulp and now I've got my theme setup.

Repositories are set on the base composer.json, so the repository in the theme wouldn't be available. And I'd still need a package.json for gulp, etc. So, at the end of the day, while perhaps useful in some small ways, I'm a little hesitant to think it will solve all dependency management issues on a site.

For the commerce example, that was done on project base. Which means that if someone requires drupal/colorbox, they still need to know to also composer require npm-asset/colorbox:^0.4. One doesn't just automatically get the npm asset, or do they?

@rodrigoaguilera
Copy link
Contributor Author

Rebased :)

For the commerce example, that was done on project base. Which means that if someone requires drupal/colorbox, they still need to know to also composer require npm-asset/colorbox:^0.4. One doesn't just automatically get the npm asset, or do they?

The extra step is still needed.

Maybe in the future contrib projects can start assuming the asset repository is added to your root composer.json just like https://packages.drupal.org/8

or since the software that manages the https://asset-packagist.org endpoint is BSD licensed it can be integrated into packages.drupal.org.

@rodrigoaguilera
Copy link
Contributor Author

I do not understand why travis fails. Can someone help with that?

@webflo
Copy link
Member

webflo commented Apr 11, 2018

@rodrigoaguilera I created a core issue to fix the failing tests. https://www.drupal.org/project/drupal/issues/2960214

composer.json Outdated
"web/libraries/{$name}": ["type:drupal-library"],
"web/libraries/{$name}": [
"type:drupal-library",
"vendor:npm-asset",
Copy link
Contributor

@mxr576 mxr576 Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather use

"type:bower-asset", "type:npm-asset"
instead. (Just as Lightning and Commerce2 do.)

README.md Outdated

For example, to use colorbox:
```
composer require npm-asset/colorbox:"^0.4"
Copy link
Contributor

@mxr576 mxr576 Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also mention chosen module and its JS dependency as an example, because if you install the library from npm-asset repo then you have to add a library specific override to the installer-paths, otherwise library gets installed with an incorrect name chosen-js.
Required line: "docroot/libraries/chosen": ["npm-asset/chosen-js"],
See:

@rodrigoaguilera
Copy link
Contributor Author

I made changes based on @mxr576 suggestions. Please review them.

README.md Outdated
chosen module expects the library at `/libraries/chosen`, but `composer require
npm-asset/chosen-js` installs the library into `/libraries/chosen-js`; the
following override installs it into the expected folder:
````json
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, one backtick too much! Sorry for that.

@rodrigoaguilera Can you quickly fix this in your branch of this PR?

Copy link

@sun sun May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Background info: This causes the closing backticks to no longer match; i.e., all following text is formatted as code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@rodrigoaguilera
Copy link
Contributor Author

Is it normal to run out of memory for php 5.6?

@rodrigoaguilera
Copy link
Contributor Author

So probably the plugins that we add increase the memory use for php 5.6.

Any ideas on how we can solve it?

From what I can gather right now is either we reduce the memory usage upstream or increase the allowed memory size for tests.

@sun
Copy link

sun commented May 11, 2018

2 of 3 builds on PHP 5.6 passed though. Could also have been a hiccup of the build platform. I do not see a button to re-test; any way to do run it again?

@rodrigoaguilera
Copy link
Contributor Author

I think the history of the PR hides it because of rebases it but it already failed twice and other PRs are not failing.

My only guess is that the composer plugin is raising the memory usage.
Maybe a PR without the plugin can clear the doubts.

Another idea is to make npm-asset and bower-asset "official" types so the plugin is not needed.
I opened an issue for that.
composer/installers#394

@sun
Copy link

sun commented May 11, 2018

A completely different question would be whether drupal-composer/drupal-project still needs to support PHP 5.6 in the first place. In my opinion, attempting to support it is a complete waste of time and resources. Today, anyone building a new Drupal site with Composer based on this project template will definitely use PHP 7+.

@rodrigoaguilera
Copy link
Contributor Author

I am almost sure the plugins drupal-project depends on do not support php 5.6.
I feel the dependency for drupal-project on php > 7.0 should be stated more clearly as is creating confusion for some folks.
#403

One of the first steps would be to remove the travis tests for php 5

@rodrigoaguilera
Copy link
Contributor Author

Good to know the memory thing was just a temporary thing.

Can we move this forward? I think it would be great for modules integrating a third party library to assume the repo is in the root composer.json and just declare a dependency.

An example of this situation:
https://www.drupal.org/project/selectize/issues/2949060#comment-12774070

@shrop
Copy link

shrop commented Sep 18, 2018

I have been working with another developer on this setup in drupal-project and it is really great. Based off the https://lightning.acquia.com/blog/round-your-front-end-javascript-libraries-composer blog post. Would love to see this land here!

@kay-o
Copy link

kay-o commented Nov 21, 2018

looks like this can be merged; anyone aware of remaining tasks? happy to help move this a step closer to being 'standard practice' if so.

@rodrigoaguilera
Copy link
Contributor Author

Are the failures of the CI related to this change?

@rodrigoaguilera
Copy link
Contributor Author

Here is an example of this workflow being adopted by a contrib module.

https://www.drupal.org/project/image_sizes

@rodrigoaguilera
Copy link
Contributor Author

I just found a Drupal issue that has the potential to simplify this PR by just allowing assets to be installed in /vendor and referencing them with a file stream wrapper.

https://www.drupal.org/project/drupal/issues/3002647

@AlexSkrypnyk
Copy link
Collaborator

This pull request/issue has been inactive for over a year and is being closed due to inactivity. If the issue still persists or the contribution is still relevant, please feel free to reopen it or create a new one.

Thank you for your understanding and your contributions to the project!

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

Successfully merging this pull request may close these issues.

Add support for installing libraries via asset-packagist