TL;DR the two systems are incompatible and are going to result in more problems and confusion than it's worth. Either the web asset API needs to be reverted, or the Document class' scripts and stylesheets properties need to be deprecated and turned into a wrapper around the asset API until the properties and methods can be fully removed. This needs to be decided before beta because by any sane developer's standards an API should be considered feature frozen and locked at that state (so pulling features should only be considered if something is critically flawed). At this point in Joomla's lifetime, you're probably better off dropping the asset API because it's too confusing and too much of a paradigm shift to be accepted naturally (and IMO the API terminology is not great and the API itself has flaws that most people aren't going to identify until it's too late to do anything about it).
Labels |
Added:
?
|
Status | New | ⇒ | Discussion |
Labels |
Added:
J4 Issue
|
Status | Discussion | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-06-29 20:09:26 |
Closed_By | ⇒ | mbabker |
I see addScript/addStylesheet as low level API,
and jHtml::script/stylesheet WebAsset as high level API,
Could just change jHtml::script/stylesheet to use WebAsset
You can only change the HTMLHelper methods with a B/C break to make that API work conceptually with assets. Which then breaks asset overloading (because the asset registry and Document API are not aware of the HTMLHelper's media override support).
The asset API is also completely incompatible with Document::addScriptDeclaration()
and Document::addStyleDeclaration()
, you just happen to get lucky and that things "work" because of the order of rendering in the Document API. The asset API has no idea how to deal with inline stuff at all.
There is a reason every time I have said there needs to be a proper asset API in Joomla that it needs to use WP_Scripts
and WP_Styles
as inspiration, and yet it seems in typical Joomla fashion instead of looking at a reliable third-party source for how to design an API the third party is ignored and a custom solution is implemented as a half-hearted thing. That API answers so many problems that you have not even begun to think about and the fact that the asset API is a detached thing from the Document only makes things worse. You need one canonical source for EVERY asset that can even be potentially loaded on a page, let alone the ones that actually do get loaded; the asset API creates two separate canonical sources and by the time you get that data into one group it's too late for anyone to do anything useful with it.
Michael's words sound scary but that's the reality. I see this Asset API as a copy/paste library put in Joomla... i can't even imagine what will happen in the real world.
@joeforjoomla everything will explode, well no
I see nothing scary.
You can only change the HTMLHelper methods with a B/C break to make that API work conceptually with assets.
yea, that what I meant
Which then breaks asset overloading (because the asset registry and Document API are not aware of the HTMLHelper's media override support).
the Assets aware about override
The asset API is also completely incompatible with Document::addScriptDeclaration() and Document::addStyleDeclaration(), you just happen to get lucky and that things "work" because of the order of rendering in the Document API.
Inline stuff was not planed to be part of asset.
About "lucky" I do not understand at all, the order stay same as it was before Asset API was introduced. It still:
styles
styles inline
scripts
scripts inline
Asset API provide a js/css files (in dependency order), and Document renders them, nothing fancy.
I have said there needs to be a proper asset API in Joomla that it needs to use WP_Scripts and WP_Styles as inspiration
I looked there and did not found much difference from what WebAsset have.
If you meant a support of inline scripts/styles, well that was not planed.
And therein lies the entire problem.
This is why I am saying either the asset API needs to go away or it needs to be re-engineered as a proper solution while there is still time to do something about it. What is in core now is not a viable solution and has only created more problems.
About "lucky" I do not understand at all, the order stay same as it was before Asset API was introduced. It still:
Maybe you should look at wp_add_inline_script()
to see why it's a bad idea for this new API to be blissfully unaware of inline scripts. Hint, the $position
parameter has a lot of power if you know how to use it.
If you mean that Asset API also should have:
addScript(file)
addStylesheet(file)
addScriptDeclaration(string)
addStyleDeclaration(string)
(and dropped in Document)
Well, that should be possible,
but this will change much in the rendering also,
also existing _scripts/_stylesheets will be ignored at all.
About possibility to place inline at different position, well, that can be tricky (especially when we tries to avoid to use of inline scripts, as much as possible).
But not that important. I never had such problem with positioning.
You can proxy properties with magic getters and setters (and in fact that
should have been done with all the leading underscore class vars to emit
deprecation notices). Which means dealing with data format consistency,
and I don’t think the structures really need to change so that shouldn’t be
an issue.
I’m feeling masochistic. I’ll give you two hours of my day off and whip up
an absolute MVP of what I feel the API should look like.
On Wed, Jul 3, 2019 at 12:29 PM Fedir Zinchuk notifications@github.com
wrote:
If you mean that Asset API also should have:
addScript(file)
addStylesheet(file)
addScriptDeclaration(string)
addStyleDeclaration(string)Well, that should be possible,
but this will change much in the rendering also,
also existing _scripts/_stylesheets will be ignored at all.About possibility to place inline at different position, well, that can be
tricky (especially when we tries to avoid to use of inline scripts, as much
as possible).
But not that important. I never had such problem with positioning.—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/issues/25309?email_source=notifications&email_token=AACZ7ILMBVNNZ45NKREGGU3P5TOWJA5CNFSM4H2Z36TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFEYZI#issuecomment-508185701,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACZ7IJ2LBETL5RX3S5ANC3P5TOWJANCNFSM4H2Z36TA
.
--
You can proxy properties with magic getters and setters
Nope, unfortunately, this will not cover every case, I already tried,
Magic getters and setters for properties does not work from inside of the context, and that exactly case of a template index.php, where $this
is a Document context.
There use of $this->_script = 'foobar';
will not trigger magic __set()
.
However running "outside" $doc->_script = 'foobar';
will trigger magic __set()
.
Unless I something missed.
Inside the class shouldn’t be relying on deprecated code unless necessary
for B/C. So you could refactor all internal class uses to non underscore
prefix and use getter to proxy outside calls to right place.
Should also work if the var isn’t actually defined in the class.
On Wed, Jul 3, 2019 at 1:51 PM Fedir Zinchuk notifications@github.com
wrote:
You can proxy properties with magic getters and setters
Nope, unfortunately, this will not cover every case, I already tried,
Magic getters and setters for properties does not work from inside of the
context, and that exactly case of a template index.php, where $this is a
Document context.
There use of $this->_script = 'foobar'; will not trigger magic __set().
However running "outside" $doc->_script = 'foobar'; will trigger magic
__set().—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/issues/25309?email_source=notifications&email_token=AACZ7IMVNYSNPXFPHPPEA3LP5TYKVA5CNFSM4H2Z36TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFL2AQ#issuecomment-508214530,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACZ7IJZY5VVK5DFIDEJK5DP5TYKVANCNFSM4H2Z36TA
.
--
Actually, I've made up my mind after looking at this again for 20 minutes and ended up writing a technical review and recommendation instead. TL;DR, pull web asset API out of 4.0 and start planning now for 5.0 to have a major paradigm shift in asset management.
https://docs.google.com/document/d/1ajboeOkOYq9Q9OdSH0d8fSa4lv6qpiDodlv0igSGZlk/edit?usp=sharing
thanks, I will check
Re-open the issue?
Status | Closed | ⇒ | New |
Closed_Date | 2019-06-29 20:09:26 | ⇒ | |
Closed_By | mbabker | ⇒ |
Status | New | ⇒ | Discussion |
so I have read :)
Well, we have a different look on the thing at some points, but in general it very close.
I look on WebAsset as high level over low level Document, and not as replacement of an asset logic.
I think this approach keep b.c. as much as possible, plus it allow (in theory) to replace WebAsset layer to anything else, that may come in future.
This comes from first implementation #8913 for J3, where was need full b.c. (or as much as possible).
Even more, the first version had all of it: addScript, addInlineStuff etc.
If people want it, then it can be as a replacement. This is not a big deal.
Another difference:
You look on AssetItem as on single file, when I as on group of files. Both thing has own pros and cons.
Most of the time I work with group of the files (load blabla.css load blabla.js). Your example with plg_installer_webinstaller
I see as group. Use of group simplify a work with layouts etc.
Current implementation of AssetItem are very flexible, it may be a single css or js, or group of files, or an empty group that just provide a dependencies. Additionally it allow to work dynamically, example add/remove files depend from language etc.
Only thing I not very like in it, is how attributes per file handled. But I did not found a better idea.
However changing to "per file" approach also an idea.
It does not simplify dependencies calculation, in fakt it will be need to loop over more items (separately per css, js). But that not a big deal.
"Per file" approach will separate css, js, inline, also may simplify positioning, and attribute managing. Yes the groups still possible (maybe as an empty AssetItem that just provide a deps to existing js/css).
This will need to separate logic per css/js then. And introduce AssetItemCss, AssetItemJs, AssetItemGroup, AssetItemInline etc. That a bit more work. But also an idea.
You not first day in Joomla community, do you believe in it? :)
Realistically saying, if it not happen now, it may never happen.
I do not see a big problem to get it done for 4.0, except time
In summary, in current implementation need to:
This will need to separate logic per css/js then. And introduce AssetItemCss, AssetItemJs,
You should not need separate data objects and if you do then you're seriously Doing It Wrong™. An Asset
DTO can be the PHP class representation of any file resource, inline snippets can be stored to the options array of that DTO. You can make an AssetGroup
DTO which is a collection of Asset
objects grouped by type and that satisfies the grouping requirement.
I look on WebAsset as high level over low level Document, and not as replacement of an asset logic.
I think this approach keep b.c. as much as possible, plus it allow (in theory) to replace WebAsset layer to anything else, that may come in future.
And that makes them incompatible. At the end of the day it is two APIs with two different approaches to solve the same problem. This causes problems, otherwise I wouldn't have to write a 6 page thesis. The asset API shouldn't be a decoupled thing from the rest of the CMS and its response handling mechanism (i.e. the Document), it should be the primary interface.
Most of the time I work with group of the files (load blabla.css load blabla.js)
That might be how workflows go, but that should not be how the API is designed. Ultimately, in its simplest definition an asset boils down to a single file. An image, a CSS file, a JavaScript file, etc. You don't write one <script>
tag which loads three CSS files and two JavaScript files into memory (well, maybe the script you're including does all that because excessive lazy loading to appease Google), you write three <link>
tags and two <script>
tags to load those resources.
This gist demonstrates the structure of a registry in the "one file is one asset" approach (no grouping logic). Inline data was not scoped, but again, the storage of it can be in the options array and you introduce a helper API to make working with it much easier (i.e. how you are only calling wp_add_inline_script()
instead of having to do all the steps of WP_Scripts::add_inline_script()
manually).
Your example with
plg_installer_webinstaller
I see as group. Use of group simplify a work with layouts etc.
Except it is not a group. Sure, the UI requires the CSS and the JavaScript to render as intended, but it is still two separate assets and it needs to be possible to independently manage both of those assets somehow. The current design of your WebAssetItem
does not facilitate that.
replace existing Document addStuff (proxy them to WebAsset), (optionally, if people want it)
You might be able to proxy the methods, but because of the public class member variables, you're screwed here. Even if there weren't the public properties to deal with, you don't have a sane way of dealing with read or write operations on those properties to bridge them from the asset data structure to the Document data structure and back again. This is why I suggest it needs to be done in one big motion, and at this point it should not be done in 4.0 because to do this in a way that doesn't result in a fractured CMS where the new API has to use hacky behavior to make things work with the old API it requires major B/C breaks and a major paradigm shift, and trying to do that change right now to land it in 4.0 is asking too much at this point.
by #25775 I have changed AssetItem from "group" to "per file".
About inline stuff.
I have examined what WP doing with inline_[script,style], and it totally different from Joomla, and has own drawbacks. It strictly linked to $handler (read Asset item), it just an option (extra data) of Asset item.
First issue you will get doing only that way:
https://wordpress.stackexchange.com/questions/298762/wp-add-inline-script-without-dependency
https://wordpress.stackexchange.com/questions/168867/using-wp-add-inline-style-without-a-stylesheet
However to implement before
/after
it is a good idea.
I think we can have both approach:
before
/after
then we add it as option to related Asset item. I think this already can be implemented in existing AssetManager, with zero b.c break as it a new feature.Status | Discussion | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-11-17 09:53:38 |
Closed_By | ⇒ | alikon |
Closed_By | alikon | ⇒ | joomla-cms-bot |
Set to "closed" on behalf of @alikon by The JTracker Application at issues.joomla.org/joomla-cms/25309
closed as we have a pr #25775
Nevermind, I'll let you all figure out how FUBAR it all is when it's too late.