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
Conversation
This works very well for me. I can recommend. |
Acquia recently released a blog post explaining this method. I hope it becomes a standard workflow. I will rebase the PR ASAP. |
I'm also using this method in my projects |
Adding |
This approach doesn't work for contrib modules right now because they can't safely require asset-packagist dependencies. Per the composer docs:
And this causes two problems:
What do you think? Is there any way around this? |
@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? |
In addition to Lightning, Drupal Commerce now ships with this approach - https://www.drupal.org/project/commerce/issues/2916058. +1 from me |
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 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 |
Rebased :)
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. |
I do not understand why travis fails. Can someone help with that? |
@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", |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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:
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Is it normal to run out of memory for php 5.6? |
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. |
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? |
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. Another idea is to make npm-asset and bower-asset "official" types so the plugin is not needed. |
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+. |
I am almost sure the plugins drupal-project depends on do not support php 5.6. One of the first steps would be to remove the travis tests for php 5 |
f09e011
to
e465af5
Compare
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: |
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! |
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. |
Be able to download download third party assets with commands like composer require bower-asset/dropzone=~4.2
Are the failures of the CI related to this change? |
Here is an example of this workflow being adopted by a contrib module. |
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. |
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! |
Be able to download download third party assets with commands like
composer require bower-asset/dropzone=~4.2
closes #278