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

[WIP] New asset management #4855

Merged
merged 15 commits into from Sep 17, 2014
Merged

[WIP] New asset management #4855

merged 15 commits into from Sep 17, 2014

Conversation

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Aug 28, 2014

Fixes #4454

Problems being addressed

Most css/js libs are not registered as Composer packages. As a result, if an extension has dependency on them, it becomes very troublesome to install the extension. As an example, the yii2-jui extension is including the whole JUI lib because there is no JUI composer package. And because of this, it is possible that two extensions are using different ways to reference the same 3rd party js lib and thus cause double inclusion of the same js file.

Current status

Basic app (including debugger and gii) can work running the following composer commands:

composer global require "fxp/composer-asset-plugin:1.0.*@dev"
composer update

Tasks to be done

  • support automatic bower/npm package installation when using composer to install an extension. This is the main blocking task currently. We plan to solve this with composer-asset-plugin.
  • enhance asset command to support asset bundle listing and asset combining/compression via grunt and other tools (may postpone this task to 2.0.1)
  • fix unit tests for asset bundles and asset manager
  • fix acceptance/functional tests for apps
  • documentation

Summary of changes

  1. Introduced @bower and @npm aliases to refer to the directories that should be used to install bower and npm packages, respectively.
  2. Removed 3rd party js libs (e.g. jquery ui, punycode) from the core codebase and rely on bower dependencies to install them when needed.
  3. Standardized the way of organizing assets, as described below.

Assets specific to application

Put the assets under a Web folder, and declare them in one or multiple asset bundle classes. For an example, see https://github.com/yiisoft/yii2/blob/new-asset/apps/basic/assets/AppAsset.php

Assets for non-redistributable widgets/modules

Put the assets under either a Web folder or the directory containing the widget/module class file. If the latter, make sure you specify AssetBundle::sourcePath so that the assets can be published when being used.

Assets for extensions

Put the assets in a directory (e.g. assets) under the extension base path. List them in a bundle class and make sure you set AssetBundle::sourcePath to be @vendor/PackageName/assets.

Third-party assets

If your application or extension uses a third-party asset package, do these steps:

  • Create an asset bundle to list the asset files to be used. Make sure you set AssetBundle::sourcePath to be @bower/PackageName, if the third-party package is a bower package (@npm/PackageName for npm packages, @vendor/PackageName for Composer packages).
  • In the asset bundle class for your application/extension, list the third-party bundle class as a dependency.
  • In composer.json of your application/extension, list the third-party package as a dependency.
qiangxue added 5 commits Aug 27, 2014
@@ -89,16 +89,16 @@ following way:
```php
class LanguageAsset extends AssetBundle
{
public $language;
public static $language;

This comment has been minimized.

@samdark

samdark Aug 29, 2014
Member

Why static?

],
"require": {
"yiisoft/yii2": "*",
"twbs/bootstrap": "3.2.* | 3.1.* | 3.0.*"

This comment has been minimized.

@samdark

samdark Aug 29, 2014
Member

Should be removed?

@@ -76,16 +76,19 @@ If you want to specify additional properties of the style tag, pass an array of
If you need to make sure there's only a single style tag use fourth argument as was mentioned in meta tags description.

```php
$this->registerCssFile("http://example.com/css/themes/black-and-white.css", [BootstrapAsset::className()], ['media' => 'print'], 'css-print-theme');
$this->registerCssFile("http://example.com/css/themes/black-and-white.css", [

This comment has been minimized.

@samdark

samdark Aug 29, 2014
Member

I like the change. Syntax is now more explanatory.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

Why it was changed overall ? Does other fw use bower ? Looks like too much steps to start the demo app

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

Amount of steps will be the same when it will be bound to Composer.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

no, now i need to install nodejs and deal with it bugs and so on , or not ?

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

It's extremely easy using both Windows, MacOS and Linux. Easier than installing Composer actually.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

i am not sure what is overall profit ? only switching composer to bower ? because i dont see any advantages except everybody use bower, can you maybe specify them in particular ?

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

It's all described in asset-related issues. But here's the summary of two main reasons:

  1. We're getting same path for the same library no matter which extension is using it. It makes situation with using two same JS or CSS files from different locations nearly impossible.
  2. It gives us a way to automate getting JS and CSS the same great way we're getting PHP packages. Composer doesn't fit there because nearly all the clientside library authors are using bower and not using Composer.
@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

So now when creating extension everyone should mention it in packagist and in bower packages ? is not it too much ?

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

Not if your extension uses third party JS instead of your own.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

so bower actually installs all packages in some other directory, and in our extendsions and widgets we should somehow link to it with aliases ? should we create @assets or something like that in application to be able to register js ?

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

Check the code.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

from where bootstrap and other comes ? I checked code but no clarification in docs or in other places .
Also how actually bower will help since you hardcoded assets like 'yii2-gii/assets/gii.js', in code ? how one can install something in yii2-gii since it is only for yii2 ?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

@qiangxue can you clarify overall advantages ( just interesting ) ? and what advantage of replacing files is in particular ?

@pmoust
Copy link
Contributor

@pmoust pmoust commented Aug 29, 2014

@Ragazzo Mark, it's a step forward, could you present any benefit in not using bower? A positive effect of sticking with previous situations, apart from we've been doing like so for a few years and it adds nodejs dependency.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

So what are real advantages and step forward is ? How it will solve problem with file conflicts and so on, since they will be named exactly as before ? We only switched composer to bower and thats it ? Also i am not against but want to see real advantages, because now it is only looks like a decision from holywar issue started by one of developers

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

Again, it was all discussed and answered:

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

@samdark the solution with

'class' => 'yii\web\AssetManager',
    'assetMap' => [
        // when seeing jquery.min.js, we should use jquery.js instead
        'bower:jquery/jquery.min.js' => 'bower:jquery/jquery.js',

        // when seeing anything matching */jquery.js, use the bower version only
        '*/jquery.js' => 'bower:jquery/jquery.js',
    ],
]

seems very hacky to me, all this unneeded prefixes and suffixes . Whatever overall

@samdark
Copy link
Member

@samdark samdark commented Aug 29, 2014

That's why current approach is different while it solves the same problems.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

Ok , thank you

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Aug 29, 2014

I am no expert at Bower, but as I see it behaves similar to Composer, placing all packages files under particular directory.
If it is so, why do we need this full-scale change? We can introduce a specific path alias named @bower or @assetsrc, which will point to the bower target directory. So all we need is specify AssetBundle::sourcePath to be @bower and specify JS and CSS relatively.
We may even mock-up bower running, composing its target directory manually for the core assets.

I don't lilke the idea of entire loosing ability to specify assets from extension local directory. This ability is very usefull while creating java script solutions, which are specific for the particular project. For example: I often create a custom widget to encapsulate complex catalog filter, which uses JavaScript and Ajax for funcy effects. During this process I create JQuery component named like 'itemFilter' and place it in the separated js file. It makes no sense for mwe to create a separated bower package to store it, because it is unlikely will be used anywhere else, but I still want my Widget to publish my js and include it to the page. Current fix makes this not possible.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Aug 29, 2014

yes , i also support @bower or other alias since it is very flexible to use with widgets and extensions in asset bundles , but @klimov-paul as was said by @samdark it is already fixed, not sure where though, maybe he will do some clarification

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Aug 29, 2014

Well, pardon we if I am wrong, but as see from the soure code of this PR AssetBundle::sourcePath is simply removed as well as entire publishing logic of AssetManager, which elminates the entire ability of publishing assets via PHP. I suppose PR leaves it all to Bower.

@qiangxue
Copy link
Member Author

@qiangxue qiangxue commented Aug 29, 2014

I updated the description of this PR trying to clarify some of your concerns. If you still have concerns, please feel free to keep discussing about them here. Yes, this change is very significant and may require change of your previous development habit, but I feel it is necessary and we are on the right direction

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Sep 2, 2014

Ah , i see your point , true and valid though

@antichris
Copy link

@antichris antichris commented Sep 2, 2014

Maybe I'm getting something wrong, but creating a bower package for every asset bundle sounds really uncomfortable to me. In my opinion the previous pattern was more sound.

Also having to convince our systems architect, that to run Yii2 we need node.js installed, because asset management has to be done via bower — does not sound like much fun.

I would not argue, that the option to manage assets via bower is a great optional feature, but, in my opinion, it ought to remain an optional feature, not one of core requirements.

@samdark
Copy link
Member

@samdark samdark commented Sep 2, 2014

@klimov-paul your use case is valid but only till the project goes production where you'll combine/compress all JavaScript to all.js and all CSS to all.css anyway. That was our initial thinking for removing runtime asset publishing.

For extensions we can add a special composer.json command that copies (or symlinks) assets at installation same way as it's now changing permissions. If there's a bower packages it could run bower command as well. It will solve a problem with immediate extension usage.

@U-D13 you can use http://bowerphp.org/ if your architect absolutely insists about avoiding node. The main reason for it is that if we'll use bower, extension authors will have proper dependencies so you won't end up with two jQueryUI included by two different extensions.

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Sep 2, 2014

your use case is valid but only till the project goes production where you'll combine/compress all JavaScript to all.js and all CSS to all.css anyway

Current implementation covers compoistion of the source files for such compression, which this PR removes.

For extensions we can add a special composer.json command that copies (or symlinks) assets at installation same way as it's now changing permissions. If there's a bower packages it could run bower command as well. It will solve a problem with immediate extension usage.

I am talking not only about extensions. I am talking about widgets and modules, which developer may create for the particular project AuthChoice is just an example of how such widget may look like. This change will enforce developer either to use inline scripts instead of *.js files, which will not allow further JS combining into "all.js", or drop the incapsulation and isolation of the particular widgets and modules creating shared throuhg entire application *.js files.

My opinion, with this PR we throwing ourthelves from one edge to another.

@samdark
Copy link
Member

@samdark samdark commented Sep 2, 2014

Any idea on how to take the best from both approaches while keeping extension authors from using common library JS/CSS from bower?

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Sep 2, 2014

I have posted it at my first enty in this disscussion:
#4855 (comment)

We may created a specific alias named @bower or something, which will point to the Bower destination directory. At all asset bundles, which uses shared libraries we can use this alias to define actual JS and CSS files.

So JqueryAsset will change from:

class JqueryAsset extends AssetBundle
{
    public $sourcePath = '@vendor/yiisoft/jquery';
    public $js = [
        'jquery.js',
    ];
}

to:

class JqueryAsset extends AssetBundle
{
    public $sourcePath = '@bower/dist/jquery';
    public $js = [
        'jquery.js',
    ];
}
@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Sep 2, 2014

Better even use baseUrl:

class JqueryAsset extends AssetBundle
{
    public $baseUrl = '@bower/dist/jquery';
    public $js = [
        'jquery.js',
    ];
}

Since ClientScript already prevents including files with duplicated URLs - all problem solved.

@samdark
Copy link
Member

@samdark samdark commented Sep 2, 2014

@qiangxue what do you think about what @klimov-paul proposes?

@antichris
Copy link

@antichris antichris commented Sep 2, 2014

@samdark I was having a discussion with our sysarchitect right while I was writing that comment, and "there's also BowerPHP," was his exact words, "but it does not work properly". BowerPHP is alpha, not something one would want to rely on in a production environment.

@klimov-paul
Copy link
Member

@klimov-paul klimov-paul commented Sep 2, 2014

@U-D13, you may compose your deployment process in a way production server will not call Bower.
You can compose bower destination folder content at the development server and then tranfer it to the production one. Thus production server may have no NodeJS and Bower installed.

Such problems can be solved.

@qiangxue
Copy link
Member Author

@qiangxue qiangxue commented Sep 2, 2014

Yes, perhaps we should bring back asset publishing because based on the feedback, it seems many developers are relying on this feature implicitly or explicitly.

We may also need to put 3rd party asset libs under vendor and use asset publishing to copy only the needed libs to the Web folder. To ensure each asset lib has a unique URL, we should enforce a rule about the location of each asset lib, e.g., vendor/bower/PackageName (for bower packages), vendor/npm/PackageName (for npm packages). If an extension contains some assets that you absolutely do not need to be extended (e.g. AuthChoice), then its source path would be its composer package path.

Regarding the dependency on bower/npm, I have evaluated bowerphp. Unfortunately, it has a lot of issues and is not usable. So now we are counting on https://github.com/francoispluchino/composer-asset-plugin/, which based on its documentation is very promising. It will eliminate the requirement of installing nodejs/bower and making installing an extension just like a regular composer package.

qiangxue added 2 commits Sep 2, 2014
@rubenheymans
Copy link

@rubenheymans rubenheymans commented Sep 3, 2014

This is awesome!

@francoispluchino
Copy link

@francoispluchino francoispluchino commented Sep 3, 2014

@creocoder for francoispluchino/composer-asset-plugin:

And main thing is this far from support of all bower features (.bowerrc, package cleanup after install, etc).

The package cleanup after install is supported for bower asset. Only bower script is not supported.

This solution will (probably) require publication since composer vendor is not under webroot.

For use the plugin in project mode (webroot), the PR composer/composer#3082 has a status 'feature' and should be merged.

Currently, the plugin has only 39 stars, which is low, but if you include the plugin in the framework, this digit will increase rapidly.

Of course I am open to any suggestions to improve the plugin.

@qiangxue
Copy link
Member Author

@qiangxue qiangxue commented Sep 3, 2014

@francoispluchino Thank you for your comment. I tried your plugin, and it worked great! I'm now eagerly looking forward to seeing its project mode support getting merged soon. This will make everything transparent to end users.

$asset = $bundle->sourcePath . '/' . $asset;
}

$n = strlen($asset);

This comment has been minimized.

@samdark

samdark Sep 3, 2014
Member

Directory may contain non-latin characters so mb_strlen should be used instread of strlen.

$opts['afterCopy'] = $this->afterCopy;
}
FileHelper::copyDirectory($src, $dstDir, $opts);
if ($this->linkAssets) {

This comment has been minimized.

@samdark

samdark Sep 3, 2014
Member

I think we should make $linkAssets true by default. It is well supported by all modern file sytems and OSes.

@cebe
Copy link
Member

@cebe cebe commented Sep 14, 2014

@qiangxue have not read it but maybe interesting information: composer/composer#3078 (comment)

qiangxue added 4 commits Sep 16, 2014
WIP
Conflicts:
	apps/basic/composer.json
	composer.json
…ugin to manage the dependencies on 3rd-party JS libraries
qiangxue added a commit that referenced this pull request Sep 17, 2014
[WIP] New asset management
@qiangxue qiangxue merged commit 999a39d into master Sep 17, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@qiangxue
Copy link
Member Author

@qiangxue qiangxue commented Sep 17, 2014

@francoispluchino FYI, yii2 master is now using your plugin to manage the asset package dependencies. So far it worked very well in my personal tests. Great job!

@samdark samdark deleted the new-asset branch Sep 17, 2014
@jiahuif

This comment has been minimized.

Copy link
Contributor

@jiahuif jiahuif commented on apps/advanced/.bowerrc in 60e07e0 Sep 20, 2014

I don't have such .bowerrc file and it just works fine. Should this file be ignored?

This comment has been minimized.

Copy link
Member

@cebe cebe replied Sep 22, 2014

this file has since been removed, no need for it.

This comment has been minimized.

Copy link
Member Author

@qiangxue qiangxue replied Sep 22, 2014

Actually this file is still there. I kept it in case users want to work with bower directly in their projects.

@RomeroMsk

This comment has been minimized.

Copy link
Contributor

@RomeroMsk RomeroMsk commented on f51a263 Sep 22, 2014

Please, add a note about depends key in options (registerJsFile/registerCssFile) to UPGRADE.md. It was surprising when assets stopped working after update.

This comment has been minimized.

Copy link
Member

@cebe cebe replied Sep 22, 2014

done 6c51866. Thanks for pointing it out.

This comment has been minimized.

Copy link
Contributor

@RomeroMsk RomeroMsk replied Sep 22, 2014

Thank you!

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

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.