yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Asset bundles approach is wrong and lead to Yii 2 extensions mess and lot of troubles #4391

Closed creocoder closed 10 years ago

creocoder commented 10 years ago

For example we have foo\yii2-gallery-widget. It require js libs:

We have 2 Asset Bundles: foo\gallery\JqueryGalleryAsset and foo\gallery\DotAsset.

Second extension we have bar\yii2-file-uploader-widget. It requre js libs:

We have 2 Asset Bundles: bar\uploader\JqueryFileUploaderAsset and bar\uploader\DotAsset.

Using this widgets in one page will lead to critical js error, because doT will be linked 2 times. Trying to use asset converter will lead to critical js error, because it will use doT 2 times. Trying to override doT bundle absolutte FAIL, because we can override 1 bundle, while there we have 2.

Because all of it troubles current Yii 2 situation is: NOT use Yii 2 extensions at all, because it just DUNGEROUS. In middle/big application you will be faced with all described troubles.

Also as you see this is simple example where trouble come. In real world extensions which requre 2 - 3 js libraries possibility of such bundle collisions is 99%.

@qiangxue It is not yet too late (there is no release yet) to totally rethink asset bundles approach. Because if it will go to release prepare to situation when yii 2 extensions will not work together and no one will use them.

creocoder commented 10 years ago

Current workaround is to avoid using Yii 2 core asset manager, any Yii 2 extensions and use 3rd party asset manager (without bundles) + extensions created for this manager. I'm sure many ppl will go that way because core idea is just NOT WORK.

qiangxue commented 10 years ago

What you should do is as follows:

  1. Create an asset bundle for each of the widgets.
  2. If you find these third party libraries available as independent asset bundles, declare them in the dependencies of your widget asset bundles.
  3. If not, create your own copy of such asset bundles encapsulating the third party libraries and list them as the dependencies of your widget asset bundles.

Now if 2 is the case, there should be no problem. If 3 is the case, in the app that uses the widgets, you can customize the widget asset bundles' dependencies to depend on unique 3rd party asset bundles.

Yes, case 3 is not good, but you will face similar problem even if you do not use asset bundle (e.g. one widget bundles its own version of 3rd party lib because it is not using bower or for some other reason).

If Composer can support bower (we already have some ideas), the problem of case 3 can be largely solved.

creocoder commented 10 years ago

Create an asset bundle for each of the widgets.

Note that i specially show that widget vendors is different! First vendor is foo. Second vendor is bar.

If you find these third party libraries available as independent asset bundles, declare them in the dependencies of your widget asset bundles.

From what time we have centralized official Yii 2 extension storage which can be trusted? I'm sure in all situations extension developer will describe asset bundle independent of it existent. Also not all js libs require any extensions. You suggest create repos for doT only to put into that repo asset bundle? Analyze of current Yii 2 extension show that this is true. Extensions which require 2+ js libraries have independent bundles for that.

If Composer can support bower (we already have some ideas), the problem of case 3 can be largely solved.

Problem i described has nothing similar to frontend package distribution. Its pure Asset Bundles concept fail. Distribution is different topic, also important. But in short mine opinion about that is: Thanks, we want use original bower, no need reinvent wheels and hack composer for distrubution. For me suggesting alternatives for that is look just strange since its N1 tool for manage frontend packages. So suggesting composer-bower bridge will be just another wrong idea.

samdark commented 10 years ago

I see what @creocoder is talking about. Indeed, it can be a problem that independent extension authors will provide their own bundle classes for the same JS library even if the library itself will be taken from the same place (for example, bower package).

qiangxue commented 10 years ago

Note that i specially show that widget vendors is different! First vendor is foo. Second vendor is bar.

I understand the widgets may be developed by different vendors. What I was describing is a recommended practice that should be taken by widget developers.

From what time we have centralized official Yii 2 extension storage which can be trusted? I'm sure in all situations extension developer will describe asset bundle independent of it existent. Also not all js libs require any extensions....

I know this topic was discussed before. Before you are so determined to discard the concept of asset bundle and fully embraces alternatives, I think it's better to compare them objectively without passion. Below are my comparisons. Perhaps you can comment on it?

https://docs.google.com/spreadsheets/d/1B3lkUCqGR6i6y-6h6YAk1VtdWXVsgm1O_dEsVe10p_M/

creocoder commented 10 years ago

@qiangxue I saw this document earlier. I absolutte disagree with that:

3). Installing a Yii extension / Composer install, bower install is Cons 4). Deploying to production / use third party tools to compress assets into a single big one is Cons

About 4 is even funny. We use 3rd party tools. Not guys who use bower. They use grunt which is again N1 tool for many things (combination/minification frontend packages is one of them).

From mine point of view using bower and manual asset management (in some central place) is faster and cleaner way than Asset Bundles. In reality Asset Bundles troubles can be solved only if we complettely remove it.

qiangxue commented 10 years ago

Because you already have a strong opinion, I feel it's hard to discuss it fairly. For me, I'm fine discarding asset bundle if there are convincing arguments against it and the alternative can solve the problems that asset bundle is designed to solve.

For 3), clearly this is a pro of asset bundle because it requires one less external environment dependency and one less command. But of course, this advantage isn't much.

For 4), can you explain if you are using JUI in some of your pages, what would you do to combine/compress asset files?

creocoder commented 10 years ago

Because you already have a strong opinion, I feel it's hard to discuss it fairly.

Yes, right. I think too much about asset bundles approach. For me its clean that this should be removed in favor of simple concept where you can manually write all your assets using some simple centralized config:

'js' => [
    'jquery.js', // all pages
    'jui.js' => ['product/create', 'product/update'], // only on pages which described by route
],
'css' => [
    'site.css',
    'selectize.css' => ['article/create'],
],

It need be better designed ofcource with taking into account config for production mode where we use 1+ minified js file and 1+ minified css file.

For 4), can you explain if you are using JUI in some of your pages, what would you do to combine/compress asset files?

If you use grunt for compression/minification you have control under all compression/minification aspects. You can compress 5 js libs into one file and 2 another js libs into second file, etc. So its not big trouble. Most important you use modern web dev workflow. Our solutions unfortunually incompatible with such workflow. Even if you want use bower for example you need hack core (override View, make registerAssetBundle() method return null, etc. You loose all yii 2 extensions as result, etc).

All i want is to make we follow modern web dev workflow. Look how another frameworks solve troubles with assets. They did not solve it at all. On other side user have full control under assets and can use any solutions. Mine opinion is its better to not have solution than have wrong. Troubles i described in first post is absolutte real and lead to incompatible yii 2 extensions.

If you still do not belive Asset Bundles is wrong concept just look at:

https://github.com/2amigos/yii2-file-upload-widget/blob/master/FileUpload.php#L62

It just for example. Such code is unacceptable because bundle lost override feature. This is another proof why asset bundle approach is complex, non error free, unusable when you want use modern web dev tools.

samdark commented 10 years ago

Let's imagine we're dropping asset bundle concept to solve "duplicate bundles from different vendors" problem. What can we do to automate things to the same level we have now?

samdark commented 10 years ago

i.e. what can we do to minimize manual edits of config when installing an extension?

creocoder commented 10 years ago

i.e. what can we do to minimize manual edits of config when installing an extension.

We need to think what we can do... it will be another solutions we can think about. Most important is avoid wrong solution inside release version. On other hands its also important stop ppl write wrong yii 2 extensions.

samdark commented 10 years ago

https://github.com/yiisoft/yii2/issues/4145#issuecomment-49656747

samdark commented 10 years ago

@creocoder understandable but we'd better spend time and release with acceptable degree of automatics rather than w/o it. Do you have ideas about how to achieve it w/o asset bundles?

qiangxue commented 10 years ago

We need to think what we can do... it will be another solutions we can think about. Most important is avoid wrong solution inside release version.

Aren't you talking about the "modern dev workflow" which is far superior and should have solved all problems?

Let's imagine we're dropping asset bundle concept to solve "duplicate bundles from different vendors" problem. What can we do to automate things to the same level we have now?

Yes, this is my opinion too. We cannot just discard a thing because you spot some problems but without giving a solution.

Here's my opinion and summary regarding the issue you raised here:

Anyway, if you could present a streamlined solution to all problems that asset bundle is addressing, I'm more than happy to take it.

SDKiller commented 10 years ago

Partial solution may be to pass some loadOptions to register function to have more flexible per-view configuration.

So if you know that 2 widgets use the same js library on the page you could pass to the second widget in loadOptions exclude array of files that will not be loaded in particular view.

Thow it does not solve the problem of in case of combined an minified assets.

creocoder commented 10 years ago

@qiangxue

Aren't you talking about the "modern dev workflow" which is far superior and should have solved all problems?

Yes this is what i talked about. And solution is simple centralized assets config like:

'components' => [
    'assetManager' => [
        'js' => ...
        'css' => ...
    ],
]

+

extension README like:

This extension requre foo js lib v2.3+ and bar js lib v3.5+ You can manually copy it to web accessable directory, you can use nmp or bower or frontend package manager which will be out after 10 yeats. Next you need add following strings to your assets config.

Simple and robust solution. Solve absolutte all troubles. You can open one file and control all your assets. You can optimize assets using any method you know. Automation is good, but not always. Sometimes it lead to complex troubles. What i write above is not joke. Currently i prefer to use this simple solution because with that you can at least write applications, use extensions. While with current solution i just sit and think long time how to use extension A with extension B, In final i decide that such extensions should burn in hell. But even in that case i need manully write asset bundles. Even if i'm ready to use js libraries manually. In that simple case i STILL NEED to write assets bundles, search and override bundles, override core classes, etc. While i just want to take and use js lib. With what i write above trouble can be solved in 10 minutes. While playing with bundles eat really long dev time with horrible results, because its looks like complex game when you fight with framework instead of just work and have fun.

samdark commented 10 years ago

@qiangxue

We do not have a globally uniquely way of identifying libraries.

We can use bower/npm packages for the purpose but even if we do it the need to declare asset bundles requires us to maintain a cenralized repos of the bundle-wrappers for each package.

Second, this issue may not be very common in practice.

It will be common when we'll have lots of extensions.

samdark commented 10 years ago

@SDKiller that's all OK if both bundles are yours but doesn't change anything when both are extensions from different vendors.

samdark commented 10 years ago

@creocoder we understand the problem and understand that it could be solved by dropping asset bundles. But it has a quite significant drawback that was already mentioned — lots of manual editing / copying. If it's possible to get the best of two approaches we'll definitely go for it.

creocoder commented 10 years ago

@samdark

lots of manual editing / copying

This is just 10 times faster than playing in "bundle" game. Really.

samdark commented 10 years ago

I'm not comparing bundles with no-bundles. I'm comparing no-bundles automated with no-bundles manual editing.

creocoder commented 10 years ago

@samdark To be honest i do not belive we can automate anything without significant drawbacks. Manual approach compatible with anything. Its long at start and fast in future. It give you total control under assets in one place. It error free.

samdark commented 10 years ago

@creocoder sure it is but we should try at least ;)

samdark commented 10 years ago

@qiangxue overall I think that while asset bundles are OK at the application level, the requirement to declare bundles could be quite fatal for Yii 2.0 extension infrastructure and code reuse.

creocoder commented 10 years ago

@samdark

overall I think that while asset bundles are OK at the application level

With bundles not all so good even on application level. On one project i faced with situation when i need 1 bundle per page due to specific js structure of bootstrap theme used. For example login page require login.js, other pages require it specific js. This lead to situation when i have LoginPageAsset depends on AppAsset, FooPageAsset depends on AppAsset, etc. It implementable. But it really 10 times longer than having central assets config. So bundles are NOT OK at application level ;)

P.S. When i start this conversation i think long about bundles and it approach. I work with it at practice long long time. My conclusion is bundles not only anti-productive, it have no benefits at all.

samdark commented 10 years ago

Right, it may take more time to deal with bundles in this case because of the need to declare all these in separate files / classes.

samdark commented 10 years ago

It seems using bower (or composer-bower) solves the problem with extensions and bundles. In this case we have files at the fixed path and it doesn't matter if these are registered by separate bundles or not. Even if there are 2 bundles, files are passed to View that prevents including same file twice. As for priorities, view renders head to body so file is automatically rendered at the uppermost position of these mentioned in bundles.

samdark commented 10 years ago

So if we'll be able to use bower packages and everyone will actually do it, there will be less reasons for dropping bundles: https://github.com/cebe/composer-bower/issues/5

qiangxue commented 10 years ago

I don't think your example about 1 bundle per page is convincing. I can have similar examples against the central list approach. For example, how would you handle conditional inclusion of certain assets? When a central list is not feasible (such as in modular development), what would you do? Basically with the central list approach you still need to solve these problems, while the asset bundle approach already solves them, although in certain cases it may take extra effort.

I think we should clarify what is indeed asset bundle. Asset bundle is a way of dividing assets into groups with dependencies. It allows you to reference and use a group of assets as a whole in your code. Asset bundle does NOT deal with asset distribution. This is done by package managers such as composer, bower. Asset bundle can be compared with your central list approach which does NOT care about asset distribution either.

Overall I don't see big problem with asset bundles.

We do have problem with asset distribution, because many js/css packages are not managed by Composer. For application-level assets, this is not a big problem because you can always manually download/install js/css packages using your preferred way. For extension-level assets, this problem is more severe, like described in this issue.

There are already some attempts to solve this problem. In the worst case when there is no Composer package for a js lib, an extension can require its users to manually download/install the js lib using bower or other tools. The user also needs to define an asset bundle with a fixed name to list the js files in the lib. This approach takes similar effort as your proposed manual management approach.

SDKiller commented 10 years ago

Central list approach with bundle approach could be combined.

There is already existing solution to avoid duplicates by registering asset files with the same key:

\yii\web\View

...
     * @param string $key the key that identifies the JS script file. If null, it will use
     * $url as the key. If two JS files are registered with the same key, the latter
     * will overwrite the former.
     */
    public function registerJsFile($url, $depends = [], $options = [], $key = null)
    {
     ...

In \yii\web\AssetBundle::registerAssetFiles this possibility actually is not utilized - key is not passed to \yii\web\View::registerJsFile - so it fallbacks to default url their:

    public function registerAssetFiles($view)
    {
        foreach ($this->js as $js) {
            if ($js[0] !== '/' && $js[0] !== '.' && strpos($js, '://') === false) {
                $view->registerJsFile($this->baseUrl . '/' . $js, [], $this->jsOptions);
            } else {
                $view->registerJsFile($js, [], $this->jsOptions);
           ...

If some kind of central list of keys could be fetched before registering assets with \yii\web\AssetBundle::registerAssetFiles, one could define there the same key for duplicate asset files from different extensions.

samdark commented 10 years ago

@SDKiller good point. If it's not the case w/ removing duplicates by keys then it needs to be changed/fixed.

samdark commented 10 years ago

Bundles are published and directories are hashes of directory name + modification time. So if file is the same it will end up in the same directory so URL will be the same so when registering assets there will be no duplicate.

samdark commented 10 years ago

So the issues left:

  1. There's certain overhead of declaring asset bundles.
  2. There's no central place one can review/edit all asset bundles.
SDKiller commented 10 years ago

...directories are hashes...

Keys should be attached to files using sourcePath of asset bundle.

Then in registerAssetFiles something like this

...
       $assetKeys = $view->getAssetManager()->getKeysList();
...
        foreach ($this->js as $js) {
            $path = Yii::getAlias($this->sourcePath . '/' .$js);
            $key = array_key_exists($path, $assetKeys) ? $assetKeys[$path] : null;
            if ($js[0] !== '/' && $js[0] !== '.' && strpos($js, '://') === false) {
                $view->registerJsFile($this->baseUrl . '/' . $js, [], $this->jsOptions, $key);
            } else {
                $view->registerJsFile($js, [], $this->jsOptions, $key);
            }
        }
...
klimov-paul commented 10 years ago

This does not solves the problem of the isssue. If 2 separated extensions uses 2 different copies of the same library thier keys still will be different.

samdark commented 10 years ago

@SDKiller doesn't matter since the same file ends up at the same URL unless configured to be different explicitly.

samdark commented 10 years ago

@klimov-paul yes but if we'll use bower packages all JS files will not be duplicated.

SDKiller commented 10 years ago

Yes, that why I'm talking about manual assignment of key for such assets in assetKeys array in AssetManager config.

SDKiller commented 10 years ago

If we know that we have problem with 2 extensions:

   'assetKeys' => [
       '@vendor/foo/abc.js' => 'abc',
       '@vendor/bar/abc.js' => 'abc',
   ]
samdark commented 10 years ago

@SDKiller there's no problem is bower packages are used.

klimov-paul commented 10 years ago

I can not see nothing critical at this issue.

Here @creocoder demands we should use central array config instead of asset bundle approach, so it can be simplier and flexible. But at the present state you can already do such thing. AssetManager supports asset bundles configuration, so you can just write the list of your files at the application config. Also you may do the same thing just editing your central AppAssetBundle source as well. Asset bundles are simmilar to regular array configuration parts. They are composed into the classes for easier registration and processing.

About bower grunt and so on. There is no restriction for developer to use 'modern approach' for asset applying. If you with to drop asset bundle usage for your application (which @creocoder asks without bringing any alternative) you can do it now without much effots. You can switch to usage of Bower or RequireJS (does anyone here remenber about it?) for your application if you need it. Just for the reference Yii introduces Active Record as database access solution, while there can be a situations when this solution is not acceptable and data mapper should be used instead. So following
this issue logic, we should just drop db active record as well, besides there is a Doctrine for it alreay.

Indeed there could be a situation, when 2 different extensions may include same js file twice. This problem existed even back in Yii1. I do not remember any complains on it back then.

Now, at example https://github.com/2amigos/yii2-file-upload-widget/blob/master/FileUpload.php#L62 code should be changed to something following:

public function registerClientScript()
{
    $view = $this->getView();

    if ($this->plus) {
        FileUploadPlusAsset::register($view); // FileUploadPlusAsset depends on FileUploadAsset
    } else {
        FileUploadAsset::register($view);
    }

We can not be responsible for the misusage of the framework by developers: whatever solution we provide they may use wrong.

klimov-paul commented 10 years ago

I do not see any actual arguments for dropping asset bundle approach. Without a doubt it has its problems, but its criticise will not help. If anyone can compose a good solid solution for its replacement, we all will be greatful.

As for bower - we have a separated issue about its usage somewhere. If it closed we can repen it and continue our discussion there.

samdark commented 10 years ago

@cebe is researching bower-composer integration. I've already pasted a link above: https://github.com/cebe/composer-bower/issues/5

creocoder commented 10 years ago

@klimov-paul

Here @creocoder demands we should use central array config instead of asset bundle approach

I do not say we need use central array config. I say we need remove Asset Bundle concept by following reasons:

Also this issue NOT ABOUT distribution. I see no any issue with distribution since there is bower or other frontend package managing tool. Moreover

We do have problem with asset distribution, because many js/css packages are not managed by Composer.

Sorry, but composer is pure PHP package manager. No need to try make it work with things which already have its own package managers which popularity more than composer several times. This just will be another wrong move.

Currently i see asset bundles as thing which will be hated by users when they will be faced with all described troubles. Personally i'm ready even use manual link and script inside layouts than use asset bundles. Because it just faster, simplier and does not have all those troubles.

P.S. Central config array is just idea. But even "no solution" is better than current approach.

samdark commented 10 years ago

@creocoder see my latest comments. It seems there's no trouble with extensions if bower packages are used with bundles.

creocoder commented 10 years ago

@klimov-paul

If anyone can compose a good solid solution for its replacement, we all will be greatful.

Remove asset bundles. Give simple manual assets config solution in central place with per routes feature. Remove all this troublemaking automation which helps only to install extensions and cause lot of troubles in future. Forget about distribution problem because it is not exists. This is good and solid. Because this solve all those troubles. Make extension developer live easier. Make applications developer live easier. Give full control under assets.

@samdark

It seems there's no trouble with extensions if bower packages are used with bundles.

How it will solve

?

klimov-paul commented 10 years ago

@creocoder, you as always don't want to listen anyone, but yourself.

If we will remove Asset Bundles and use 'simple manual assets config solution', what will become with all core widgets such as ActiveForm, GridView and so on? ActiveForm js depends on JQuery, which is composed into asset bundle you hate so much. At the moment this solution allows several differnt widgets to use JQuery library, without bothering about its duplicating on the page. So what do you suggest, that we should write a long manual for all built-in widgets telling how developer should compose 'manual' config to make them work?

And this what you call better solution?

My resolution for your proposal is simple: don't like asset bundles - don't use them.

samdark commented 10 years ago

@creocoder

troubles with situation when application require one bundle per one page

Where's the trouble? Just register bundles required at pages you need.

troubles with using original bower / any package manager

Most probably we'll use https://github.com/francoispluchino/composer-asset-plugin that solves the issue (uses native bower/npm when available etc.). At least currently it looks like the best option.

troubles where all those bundles games eat a lot more time than even manual link and script inside layout

Is it because it takes time to create class-wrappers?

creocoder commented 10 years ago

@klimov-paul

My resolution for your proposal is simple: don't like asset bundles - don't use them.

I'm out of this discussion. Lets wait real troubles after release. Mine vision is Asset Bundles biggest Yii 2 mistake which will be huge blow to popularity. As i say earlier i have good experience with asset bundles. Also i saw another peoples (novices) troubles with that approach ;) Still think automation needed? Still think there is no troubles with extensions? Ok, np...

SDKiller commented 10 years ago

@samdark

Bower-related solutions may be huge improvement. But I still think current implementation of \yii\web\AssetBundle::registerAssetFiles wrapper reduces flexibility in comparison with native \yii\web\View::registerJsFile() and \yii\web\View::registerCssFile()

Could you review this? https://github.com/SDKiller/yii2/pull/1/files