? Pending

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
31 Dec 2018

Summary of Changes

The Topological sorting for WebAsset works good, but can have a bit random order between non dependent items.
With a weight we can control that.

@infograf768 already found related issue, when template.css not always at "bottom"
See his comment #22263 (comment)

I have set default weight for a template assets to 100000 so it should be always at bottom, unless someone want it more down.

Testing Instructions

Apply the patch and make sure all still working as before.

Additionally inspect the source of /administrator/index.php?option=com_content page,
make sure that template.css at bottom (not before choices.css).

@infograf768 please test ?
ping @wilsonge

Note: The patch does not always set the weight "exact to requested" but tries to do it as close as possible, to keep the dependency sorting.

For reference #22435

avatar Fedik Fedik - open - 31 Dec 2018
avatar Fedik Fedik - change - 31 Dec 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 31 Dec 2018
Category Administration Templates (admin) Repository Libraries Front End Templates (site)
avatar Fedik Fedik - change - 31 Dec 2018
The description was changed
avatar Fedik Fedik - edited - 31 Dec 2018
avatar Fedik
Fedik - comment - 1 Jan 2019

Can we limit ourselves to between 0 and 100 please (or something slightly more sane than 100000). Make templates something like priority 95 - so people can use plugins to insert something later if they really want to

hm, I not very like a limits, and 100 (even 1000) is a very strict limit

I had thought about to allow only a positive weight and forbid a negative, but the code works with integer no matter what number there (positive or negative) so I have drop this idea.

I tried to keep integers and I have made a step 10 to have an enough span if someone want to change the weight, without involving of crazy float numbers like 0.0001 etc.

$activeAssets[$name]->setWeight($index * 10);

If the page have 10 assets, then heaviest weight already will be 90 (starting from 0) , that why I am not like a limits. ?

slightly more sane than 100000

On first I had set 1000, then changed to what you see, to be sure, in case the page has 100 assets (who know how people will use it if will use ? )

In the result it just a numbers that help to arrange the assets in output, nothing more.

avatar Fedik Fedik - change - 1 Jan 2019
The description was changed
avatar Fedik Fedik - edited - 1 Jan 2019
avatar Fedik
Fedik - comment - 1 Jan 2019

what I can suggest:

  • make initial weight starting from 100 (to have a span on "front" without involve of negative number)
  • a weight step stays 10, so it will be for 3 active assets: 100, 110, 120
  • set the weight to Core asset to 50, so it will stay at top of the tree
  • reduce default template weight to 10000, so it still stay at bottom of the tree

@wilsonge what do you think about it?

Technically it just a cosmetic changes to make it look nicer ?
It does not affect the calculation algorithm.

avatar mbabker
mbabker - comment - 1 Jan 2019

Really, this web asset "registry" thing is becoming over complicated.

  • There should be a difference between known (registered) assets and assets scheduled to be included on the page (current addScript() and addStyleSheet() in the Document API), right now there isn't; everything is either known to the registry and being spit out on the page or it's not there
  • Weighting should not be an API capability at all; if the registry, or whatever is actually managing assets scheduled to be included on the page (BTW this isn't a registry in the industry accepted naming of one), knows how to sort things and everything can actually state what its dependencies are, then weighting is absolutely not necessary. Weighting and declaring dependencies are not compatible concepts.
  • Random JSON files, really? There should be ONE core manifest somewhere that registers ALL of the core scripts to the registry (not this random "load file here and load file there" behavior that seems to be in place now), and ONE manifest for each extension package (though you really shouldn't even need JSON manifests at all, just use a PHP API and stop trying to get fancy)
  • Really, look at WordPress; specifically the WP_Dependencies class and its WP_Scripts and WP_Styles children, and the core function that handles registering all of the core assets to the dependency managers (those classes I just mentioned), why does Joomla need something more complex than that?

Realistically Joomla needs a proper dependency manager. Right now I seriously wouldn't use this.

avatar Fedik
Fedik - comment - 1 Jan 2019

There should be a difference between known (registered) assets and assets scheduled to be included on the page ....

Sorry, I not very understood this point,
getActiveAssets() - is "get assets scheduled to be included".
Only getKnownAssets not exists in way you described.

Random JSON files, really? There should be ONE core manifest somewhere that registers ALL of the core scripts to the registry ...

yeah, but you remember that we have /media that comes from 10 random places: /vendor, /legacy, /system etc ?

not this random "load file here and load file there" behavior that seems to be in place now

it has an order: core (/vendor, /system, /legacy), then component, then template

BTW this isn't a registry in the industry accepted naming of one

can rename to whatever, what people like, I do not mind the naming

Weighting and declaring dependencies are not compatible concepts.....

Sorry but I am do not agree, and this pull request confirm that it IS compatible.
And it even useful, maybe a bit to complicated to understand, but it IS working.

Really, look at WordPress; specifically the WP_Dependencies class and its WP_Scripts and WP_Styles children ....

what is their simplicity? from your point of view ?

Joomla needs a proper dependency manager. Right now I seriously wouldn't use this.

You got an incorrect result or why it is not a proper? it a bit hard to understand to me


Well, what I tries to achieve is consistent result, and easy to use:

  1. Consistent result: no mater where asset was enabled (in module or in component or in template), it always have the same position in output.
  2. Easy to use: you define list of your assets and use it in the same way for any asset: >enable('asset')
    Your asset can contain both JS and CSS file, or only one of them, you do not worries about it and do not worries where it located for JHtml::_('system/blabla.js') or JHtml::_('system/legacy/blabla.js'), you just do ->enable('blabla').
    This is clean up the layouts, and make their "code support" more easy, in case if the asset location changes.
avatar mbabker
mbabker - comment - 1 Jan 2019

There should be a difference between known (registered) assets and assets scheduled to be included on the page ....

Sorry, I not very understood this point,
getActiveAssets() - is "get assets scheduled to be included".
Only getKnownAssets not exists in way you described.

The registry should be all the known assets. The enqueued (scheduled to be rendered) assets should be tracked somewhere else, probably as a property or class attached to the Document itself. The registry can be singleton for the entire application (as in a singleton service in the DI container, not singleton in the Joomla 3 line of thinking), as it is just a resource that tracks what assets the application is aware of and knows nothing about scheduled inclusions, overrides, or any of that stuff. The list of scheduled assets needs to be on a per-Document basis, as remember when an error page is rendered a new Document is created so as to not be "polluted" with changes from the main execution path. The registry should be seeded, somehow, with the known core assets. Whether this is a JSON file or PHP API does not matter, but the registry should start by knowing about everything in core. #17328 was a decent example of a good registry solution, including pre-seeding with core services, before the component architecture started being rewritten (since Joomla still operates under an extreme lazy load system design where you have to still manually load everything it's honestly still painful to create global services and register global dependencies, hell you still need an onAfterInitialise listener to make a library extension available globally because there is no "boot library extensions" step in the Joomla application meaning those extensions are about as useful as files extensions).

Weighting and declaring dependencies are not compatible concepts.....

Sorry but I am do not agree, and this pull request confirm that it IS compatible.
And it even useful, maybe a bit to complicated to understand, but it IS working.

You're forcing it. Just because it works does not mean it is right. Declaring your dependency chain is for all intents and purposes weighting assets. The fact you also have to manually specify weighting in addition to declaring your dependencies means that your dependency resolution algorithm is flawed.

Really, look at WordPress; specifically the WP_Dependencies class and its WP_Scripts and WP_Styles children ....

what is their simplicity? from your point of view ?

This is clean and simple.

	$suffix = defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ? '' : '.min';

	wp_enqueue_script( 'my-theme-popper', get_theme_file_uri( "js/popper$suffix.js" ), [], '1.14.6' );
	wp_enqueue_script(
		'my-theme-bootstrap',
		get_theme_file_uri( "js/bootstrap$suffix.js" ),
		[
			'jquery',
			'my-theme-popper',
		],
		'4.2.1'
	);

I name the asset, specify the URI to the file (as WordPress doesn't have media overrides the same as Joomla that part isn't an issue, but realistically this isn't too far different from the $file argument of HTMLHelper::script() or HTMLHelper::stylesheet() if you really need file override support), declare the asset's dependencies, and give an asset version for the versioning query string.

2. jquery-noconflict has dependency from jquery, but topology sorting can produce: jquery, foo.js, bar.js jquery-noconflict.js, but it is better when jquery-noconflict goes directly after jquery

Here's an idea. Stop writing scripts that require noconflict support (seriously Joomla is the only place I have ever used that). Or, if you're really dependent on it, the declare noconflict as a dependency. Stop trying to write over-engineered "smart" code to deal with edge cases because someone can't be bothered to do something right.

avatar mbabker
mbabker - comment - 1 Jan 2019

Missed this part, so sorry for the quick follow on response.

  1. Consistent result: no mater where asset was enabled (in module or in component or in template), it always have the same position in output.

Proper dependency management handles this too. Weighting is an unnecessary extra step that the extension developer has to take (because they then have to weight their scripts against both core and the multitude of scripts in the ecosystem to get their scripts in just the right spot) and is an unnecessary extra calculation the core API has to make. Core should only need to sort things if it reaches a point where it is rendering two or more scripts with the same parent dependencies; if foo.js and bar.js are both dependent on core.js and jquery.js (with noConflict for the sake of argument), then FIFO ordering takes care of the rest and whichever is registered first between foo.js and bar.js comes first in the document. If foo.js has a dependency on bar.js then that should be declared otherwise it really doesn't matter if foo.js or bar.js is rendered first.

2. Easy to use: you define list of your assets and use it in the same way for any asset: >enable('asset')

Your definition of "easy to use" doesn't align with mine ?

Sure, $registry->enable('asset'); might make sense for a quick one-liner in the PHP API. How do I get things into the registry? How do I know what I can enable? What happens if something is disabled, does that mean its dependencies are also disabled or the dependencies still go but without something they rely on? How do I know something is enabled and set for inclusion on the page versus something that's just available for use but not included on the page? If I as a developer want to override an asset how do I do that? You say one of these assets can register multiple files, what if I only want to do something with a single file in that asset (i.e. what if I want to get rid of the noConflict stuff in the jQuery asset?)? Considering it's pretty painful to write any form of tests for J4, the only thing anyone following along has to work from is the existing API design and whatever ideas are floating around in your head, there's nothing demonstrating how the API is supposed to work or what behaviors core is or is not supporting.

avatar Fedik
Fedik - comment - 2 Jan 2019

The registry should be all the known assets. The enqueued (scheduled to be rendered) assets should be tracked somewhere else, probably as a property or class attached to the Document itself....

Okay I understood now, what you meant.
For now it "all in one", look at it not like WebAssetReqistry but like WebAssetAllInOne ?

The registry can be singleton for the entire application
The list of scheduled assets needs to be on a per-Document basis, as remember when an error page is rendered a new Document is created so as to not be "polluted"

Hm, I did not thought about it, seems a valid issue. Especialy when need multiple Document instances: regular and error doc.

To split WebAssetAllInOne need some more work. Need to think about, but not in current pull.

The fact you also have to manually specify weighting in addition to declaring your dependencies means that your dependency resolution algorithm is flawed.

I think we have diferent meaning what is dependency ?
Well, you cannot define dependency for everithing, that make no sense.
Look on original issue #22263 (comment) (The independed choices.css loaded after template.css)
From your idea it means that we have to define ALL active assets as dependency to "template" asset to be sure it will be at bottom. For this we have to loop ALL active asset and build a fake dependecny to "template" asset.
But it just useless work, we can just set a "weight in the tree" and then the template will be moved down in the dependecny tree (and so placed after independed choices.css and other such things).

That is more smart from my point of view. It is working and it is do not break dependecy tree.
There simple rule in the weight code: the asset cannot be lighter than its parent. This not require much calculation.

// Check the predecessors (Outgoing vertexes), the weight cannot be lighter than the predecessor have
$topBorder = $weight - 1;
if (!empty($graphOutgoing[$item]))
{
$prevWeights = [];
foreach ($graphOutgoing[$item] as $pItem)
{
$prevWeights[] = $activeAssets[$pItem]->getWeight();
}
$topBorder = max($prevWeights);
}

This is clean and simple.
wp_enqueue_script(...
wp_enqueue_script(...

Nooo, that not clean and simple, that old and exactly what I trying to avoid ?
It not far away from what we have, except we do not have "naming" and "dependency":

JHtml::_('jquery.freamwork');
JHtml::_('script', 'popper.js');
JHtml::_('script', 'bootstrap.js');

In wordpres aproach you still must know all dependecy, load ALL scripts manualy, and copy exactly same code around 10 layouts.
That exactly what I trying to avoid.

Stop writing scripts that require noconflict support

I just trying to keep old behavior, I do not realy care about noconflict support. I know one guy who happily wil remove all jQuery ?

Proper dependency management handles this too. Weighting is an unnecessary extra step that the extension developer has to take

As you can see from my previos point about the issue, dependency management cannot handle all, unless we define dependency for everything, that as I said does not make much sense from my point of view.
I do not force anyone to use it. It just thing that exist, I just add possibility to control it in its own borders.

Your definition of "easy to use" doesn't align with mine

That exactly what I thought, realy different ?
As I see you just like to controll everything.

How do I get things into the registry? How do I know what I can enable? If I as a developer want to override an asset how do I do that?

That need to be documented of course, someday ?
Overide stuff you already can see in template asset.json.

What happens if something is disabled, does that mean its dependencies are also disabled or the dependencies still go but without something they rely on?

Two case:

  1. you disable previously enabled asset: yes all dependecy will be disabled, except dependecy that used by another asset.
  2. you cannot disable an asset that was enabled as dependesy to one of enabled asset.

How do I know something is enabled and set for inclusion on the page versus something that's just available for use but not included on the page?

it just $existAndActive = $registry->getAsset('blabla') && $registry->getAsset('blabla')->isActive();

You say one of these assets can register multiple files, what if I only want to do something with a single file in that asset (i.e. what if I want to get rid of the noConflict stuff in the jQuery asset?)?

Your asset should contain only required files for this asset, and split down for multiple asset if it has multiple meanings.
Example "jquery" and "jquery-noconflict" is a 2 separated assets, where last one depend from first one.
Another example is Bootstrap, that split down to "bootstrap.css" and "bootstrap.js", that allow to use bootstrap styles without require of bootstrap javascript.

avatar Fedik Fedik - change - 2 Jan 2019
Labels Added: ?
avatar mbabker
mbabker - comment - 2 Jan 2019

Look on original issue #22263 (comment) (The independed choices.css loaded after template.css)
From your idea it means that we have to define ALL active assets as dependency to "template" asset to be sure it will be at bottom. For this we have to loop ALL active asset and build a fake dependecny to "template" asset.
But it just useless work, we can just set a "weight in the tree" and then the template will be moved down in the dependecny tree (and so placed after independed choices.css and other such things).

Follow FIFO rules, whatever is enqueued first is rendered first. If choices.css is being enqueued after the template's CSS file, then correctly it should be rendered after the template's CSS file. Trying to create an arbitrary "templates/<template>/css/template(.min).css is always the second to last CSS file to load (last being that user.css file)" rule is over-complicating things. Yes, that does potentially mean some CSS can be loaded after the template and could override the template, but that is the way it should be if you have two resources that for all intents and purposes have no dependencies. Managing dependencies is really not all that hard, creating special rules to deal with special cases or arbitrary decisions makes things hard.

This is clean and simple.
wp_enqueue_script(...
wp_enqueue_script(...

Nooo, that not clean and simple, that old and exactly what I trying to avoid ?
It not far away from what we have, except we do not have "naming" and "dependency":

JHtml::_('jquery.freamwork');
JHtml::_('script', 'popper.js');
JHtml::_('script', 'bootstrap.js');

In wordpres aproach you still must know all dependecy, load ALL scripts manualy, and copy exactly same code around 10 layouts.
That exactly what I trying to avoid.

You have to know all the dependencies with this "registry" too. The difference is you are using JSON manifests, WordPress is entirely PHP API driven.

BTW, in whatever function(s) you have hooking WordPress' wp_enqueue_scripts action, you can register scripts without enqueuing them, then the only bit that you "copy exactly the same code around 10 layouts" is the enqueuing line.

// In my theme's functions.php
add_action(
	'wp_enqueue_scripts',
	function () {
		$suffix = defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ? '' : '.min';

		wp_register_script( 'my-theme-popper', get_theme_file_uri( "js/popper$suffix.js" ), [], '1.14.6' );
		wp_register_script(
			'my-theme-bootstrap',
			get_theme_file_uri( "js/bootstrap$suffix.js" ),
			[
				'jquery',
				'my-theme-popper',
			],
			'4.2.1'
		);
	}
);
// In one of my custom page templates
wp_enqueue_script('my-theme-bootstrap');

Take a look at the WordPress wp_default_scripts() function. This is the function registering all of the core scripts to its registry without making them all loaded on the page, you still have to enqueue the ones you want to use. This is why it's important to have a separation between the registry of all assets and what is actually supposed to be included on the page (note WordPress mixes the registry and scheduled for rendering responsibilities in the WP_Dependencies class and its child subclasses, but it is two separate responsibilities and should be two separate APIs working closely together).

namespace Joomla\CMS\WebAsset;

interface WebAssetRegistryInterface
{
	public function add(WebAssetItem $asset): self;

	/**
	 * @throws  \Joomla\CMS\WebAsset\Exception\UnknownAssetException
	 */
	public function get(string $assetName): WebAssetItem;

	public function remove(WebAssetItem $asset): self;
}

namespace Joomla\CMS\Document;

use Joomla\CMS\WebAsset\WebAssetItem;

/**
 * Describes an object which manages assets for a Document
 */
interface WebAssetManagerInterface
{
	public function attachToDocument(Document $document): void;

	/**
	 * Disables an asset, presumably this will remove it from the manager and not only change its internal state, details to be decided later
	 */
	public function disableAsset(AssetItem $asset): self;

	public function enableAsset(AssetItem $asset): self;

	/**
	 * @return  WebAssetItem[]
	 */
	public function getAssets(): array
}
avatar Fedik
Fedik - comment - 2 Jan 2019

Follow FIFO rules, whatever is enqueued first is rendered first.
If choices.css is being enqueued after the template's CSS file, then correctly it should be rendered after the template's CSS file....

Keeping FIFO also complicated ?
For this I have to add an internal counter to keep track when last asset was enabled, and use this counter as (weight + 1) for next enabled asset (to apply FIFO order after assets was sorted by dependency). So the weighting will be involved anyway, in one or another way ?
One profit from this that I do not need to define a weight in the JSON registry (but still allowed).

Well, but after splitting to WebAssetRegistry and WebAssetManager, a loot can change.

You have to know all the dependencies with this "registry" too

yes and no, but mainly, you just need to know which asset do you need and whether it exists. All dependencies will be picked up automatically.

Take a look at the WordPress wp_default_scripts() function ...

It very close to what I did.
But I have use enable/disable (because I have a heap ?) instead of enqueue/dequeue (because they have split the thing).

And I have use JSON because it more easy to generate vendors > asset.json, and I thought it more easy for template developers than our JHtml::_('foobar.freamwork').

WebAssetRegistryInterface / WebAssetManagerInterface

That looks good.
One thing I am wories:

  1. WebAssetItem object, that I guess need to be cloned before add to Manager.
  2. Overide an existing WebAssetItem currently very easy:
    public function addAsset(WebAssetItem $asset): self
    {
    // Check whether the asset already exists, so we must copy its state before override
    if (!empty($this->assets[$asset->getName()]))
    {
    $existing = $this->assets[$asset->getName()];
    $asset->setState($existing->getState());
    }
    $this->assets[$asset->getName()] = $asset;

    But after splitting to Registry/Manager, the overide will happen on Registry level, and so I guess the Registry should notify all active Managers about override.
avatar Fedik
Fedik - comment - 2 Jan 2019

@wilsonge for now I have reduce default weight of template asset, and make weight start from 100 with step 10,
Also I have removed "noconflict" weight, and negative weights from json.

avatar Fedik
Fedik - comment - 3 Jan 2019

I almost got working WebAssetRegistry / WebAssetManager, need some time to get it in shape.

I am closing here for now.

avatar Fedik Fedik - change - 3 Jan 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-01-03 19:31:03
Closed_By Fedik
avatar Fedik Fedik - close - 3 Jan 2019

Add a Comment

Login with GitHub to post a comment