search

yiisoft / yii-jquery

Yii Framework jQuery Extension
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
27 stars 12 forks source link

reloadableScripts property and initScriptFilter() function don't fully reflect their purpose in yii.js #16

Open arogachev opened 7 years ago

arogachev commented 7 years ago

I found this issue during writing tests for yii.js.

After this change - https://github.com/yiisoft/yii2/commit/8809d422dced06337a89ea1533c564eb5628d574 along with JS we additionally handle CSS too, so reloadableScripts and initScriptFilter() are not appropriate names. I already renamed initScriptFilter() to initAssetFilters() in the PR containing tests for yii.js (additionally changed filter to filters because there are 2 separate filters now set in the same function).

But this is not a big deal, because this function is private. However we have related reloadableScripts configurable property. This should become something like reloadableAssets by analogy. However this is a little BC break. Is it OK to apply such change for JS and document in UPGRADE or it only can be done in 2.1?

Note: I already partially fixed it in yii.js in PR containing tests for yii.js. PR will be sent soon.

arogachev commented 7 years ago

The alternative is to split each into 2 dedicated properties / methods. With methods it's OK, but as for property - I think it's better to have just one because the purpose is the same. But in case of splitting property it's a BC break. Leaving property as is and splitting just methods does not solve problem for property.

samdark commented 7 years ago

2.1 and a single property, I think...

arogachev commented 7 years ago

@samdark It's fixed for method: initScriptFilter() -> initAssetFilters(), but for the property reloadableScripts is still actual. We can't just rename it to reloadableAssets now because it's a BC breaking change. Please reopen.