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

Open
wants to merge 6 commits into
base: 8.x
from

Conversation

Projects
None yet
@rodrigoaguilera
Copy link
Contributor

rodrigoaguilera commented Jun 25, 2017

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

closes #278

@fidelix

This comment has been minimized.

Copy link

fidelix commented Aug 23, 2017

This works very well for me. I can recommend.

@rodrigoaguilera

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented Aug 23, 2017

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

This comment has been minimized.

Copy link

FatherShawn commented Oct 26, 2017

I'm also using this method in my projects

@hawkeyetwolf

This comment has been minimized.

Copy link

hawkeyetwolf commented Oct 27, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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 rodrigoaguilera force-pushed the Ymbra:8.x branch from f5e6fde to 775035e Apr 6, 2018

@rodrigoaguilera

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented Apr 6, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented Apr 6, 2018

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

@webflo

This comment has been minimized.

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

"web/libraries/{$name}": ["type:drupal-library"],
"web/libraries/{$name}": [
"type:drupal-library",
"vendor:npm-asset",

This comment has been minimized.

@mxr576

mxr576 Apr 25, 2018

Contributor

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"

This comment has been minimized.

@mxr576

mxr576 Apr 25, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented May 1, 2018

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

This comment has been minimized.

@sun

sun May 2, 2018

Dang, one backtick too much! Sorry for that.

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

This comment has been minimized.

@sun

sun May 2, 2018

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

This comment has been minimized.

@rodrigoaguilera

rodrigoaguilera May 2, 2018

Author Contributor

Done!

@sun

sun approved these changes May 3, 2018

@rodrigoaguilera

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented May 3, 2018

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

@rodrigoaguilera

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented May 10, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented May 11, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented Jun 25, 2018

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 added some commits Jun 25, 2017

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

This comment has been minimized.

Copy link
Contributor Author

rodrigoaguilera commented Sep 14, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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.

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