[4.0] WebAsset allow to set a custom weight when it possible, and speed up checking for new files#23415
[4.0] WebAsset allow to set a custom weight when it possible, and speed up checking for new files#23415Fedik wants to merge 4 commits intojoomla:4.0-devfrom
Conversation
Try to set a requested weight, or make it close as possible to requested, but keep the Graph order
| } | ||
|
|
||
| // Update a weight for each active asset | ||
| $requestedWeights = []; |
There was a problem hiding this comment.
Rather than running through like this can we not use an SplPriorityQueue? (http://php.net/manual/en/class.splpriorityqueue.php)
There was a problem hiding this comment.
Sounds like a good idea, I need to check
There was a problem hiding this comment.
hm, no, SplPriorityQueue not really helpful,
I just need key (string) => value (number) array, in already defined order
| "media/system/js/core.min.js" | ||
| ] | ||
| ], | ||
| "weight": -1 |
There was a problem hiding this comment.
that because weight starts from 0 https://github.com/joomla/joomla-cms/pull/23415/files#diff-fdfafc5c384699980616dda96e6b08bfL492
it was quick idea to make it -1 😉
There was a problem hiding this comment.
You shouldn't need weighting, especially as a public API feature, if everything can properly declare all dependencies and the code handling registration enabling assets can sort things correctly 😉
There was a problem hiding this comment.
if everything can properly declare all dependencies
yeah, that true,
but I noticed 2 things, I wanted to fix here:
- make core.js always at top: this not always work, because example jquery, do not have core as dependency, so sometimes it goes before the core.js, the same can say about other "independed" scripts
- 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
and I think the weight is what can help here, without forcing a fake dependency
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. If the page have 10 assets, then heaviest weight already will be 90 (starting from 0) , that why I am not like a limits. 😉
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. |
|
what I can suggest:
@wilsonge what do you think about it? Technically it just a cosmetic changes to make it look nicer 😉 |
|
Really, this web asset "registry" thing is becoming over complicated.
Realistically Joomla needs a proper dependency manager. Right now I seriously wouldn't use this. |
Sorry, I not very understood this point,
yeah, but you remember that we have /media that comes from 10 random places: /vendor, /legacy, /system etc 😉
it has an order: core (/vendor, /system, /legacy), then component, then template
can rename to whatever, what people like, I do not mind the naming
Sorry but I am do not agree, and this pull request confirm that it IS compatible.
what is their simplicity? from your point of view 😉
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:
|
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
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.
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
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. |
|
Missed this part, so sorry for the quick follow on response.
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
Your definition of "easy to use" doesn't align with mine 😉 Sure, |
Okay I understood now, what you meant.
Hm, I did not thought about it, seems a valid issue. Especialy when need multiple Document instances: regular and error doc. To split
I think we have diferent meaning what is dependency 😉 That is more smart from my point of view. It is working and it is do not break dependecy tree. joomla-cms/libraries/src/WebAsset/WebAssetRegistry.php Lines 502 to 512 in 5ad8ec8
Nooo, that not clean and simple, that old and exactly what I trying to avoid 😉 In wordpres aproach you still must know all dependecy, load ALL scripts manualy, and copy exactly same code around 10 layouts.
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 😄
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.
That exactly what I thought, realy different 😉
That need to be documented of course, someday 😄
Two case:
it just
Your asset should contain only required files for this asset, and split down for multiple asset if it has multiple meanings. |
Follow FIFO rules, whatever is enqueued first is rendered first. If
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' // 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 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
} |
Keeping FIFO also complicated 😄 Well, but after splitting to WebAssetRegistry and WebAssetManager, a loot can change.
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.
It very close to what I did. 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').
That looks good.
|
|
@wilsonge for now I have reduce default weight of template asset, and make weight start from 100 with step 10, |
|
I almost got working I am closing here for now. |
4 participants
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_contentpage,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