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

[4.0] Deprecate Document scripts/stylesheets or revert web asset API #25309

Open
mbabker opened this issue Jun 23, 2019 · 18 comments

Comments

@mbabker
Copy link
Member

commented Jun 23, 2019

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).

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

Nevermind, I'll let you all figure out how FUBAR it all is when it's too late.

@mbabker mbabker closed this Jun 29, 2019

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

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.

@joeforjoomla

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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.

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

@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.

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

And therein lies the entire problem.

  • The web asset API is a detached API from the Document API, therefore there is no longer a canonical source of assets in a page load (and screw the idea that it could have a registry of every known asset)
  • The web asset API only manages "files" as assets, it does take into consideration inline resources
  • It forces developers to pick and choose which system they are to use ("conventional" Document/HTMLHelper or new API)

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.

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

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.

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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.

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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.

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

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

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

thanks, I will check

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Re-open the issue?

@HLeithner HLeithner reopened this Jul 5, 2019

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

so I have read :)

Well, we have a different look on the thing at some points, but in general it very close.

WebAsset as layer or as replacement

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.

AssetItem "group" or "per file"

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.

Waiting for 5.0

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 :neckbeard:

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

In summary, in current implementation need to:

  • change AssetItem from "group" to "per file" (if people want it)
  • add support of inline stuff
  • replace existing Document addStuff (proxy them to WebAsset), (optionally, if people want it)
@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

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.

@Fedik

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

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:

  • If inlineScript/Style need to be 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.
  • if it a Independent inlineScript/Style then it become a "stand alone" Asset item and will be rendered as Joomla! doing it for 100 years. This a bit more tricky, we easier leave all as is, or proxy addDeclaration to AssetManager, and just ignore existing _script/_style properties (I think it not a critical b.c. break)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.