Feature PR-5.2-dev Pending

User tests: Successful: 0 Unsuccessful: 0

avatar Fedik
Fedik
20 Jan 2024

Summary of Changes

Implementing cross-dependencies for assets.
Means now the script asset can have a style asset in dependency, and vise versa (but not in the loop of course).

@wilsonge @HLeithner what do you think?

Example:

{
  "name": "foo",
  "type": "style",
  "uri": "foo.css"
},
{
  "name": "bar",
  "type": "style",
  "uri": "bar.css"
},
{
  "name": "bar",
  "type": "script",
  "uri": "bar.js",
  "crossDependencies": ["bar#style"]
},
{
  "name": "foo",
  "type": "script",
  "uri": "foo.js",
  "dependencies": ["bar"],
  "crossDependencies": ["foo#style"]
},

Here calling $wa->useScript('foo') will enable all 4 assets.

Testing Instructions

Apply patch, run npm install,
Edit article, and use tag field.
Edit module, and use module position field.
Use smart search with autocomplete enabled.

Actual result BEFORE applying this Pull Request

Works

Expected result AFTER applying this Pull Request

Works

Link to documentations

Please select:

References:

avatar Fedik Fedik - open - 20 Jan 2024
avatar Fedik Fedik - change - 20 Jan 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Jan 2024
Category Administration com_categories com_modules com_templates Repository Front End com_finder Layout Libraries Modules
avatar HLeithner
HLeithner - comment - 20 Jan 2024

I think that make it much less verbose and unnecessary bloated. We have one drawback if you don't want the style you have to override the style which might not be a problem.

Is introducing a cross dependency property really needed? wouldn't it be possible to add array syntax to dependency key?

avatar Fedik
Fedik - comment - 20 Jan 2024

wouldn't it be possible to add array syntax to dependency key?

This will make code much more complicated and probably slower, also b/c is questionable.
Separate property is more clean and simple.

avatar HLeithner
HLeithner - comment - 20 Jan 2024

This will make code much more complicated and probably slower, also b/c is questionable. Separate property is more clean and simple.

not so sure if it would be significant slower than parsing json already :-)
For me it's more our the developer experience, we will get questions when do I use what and why shouldn't I write everything into the crossDependencies key

            "crossDependencies": {
              "style": ["choicesjs"],
              "script": ["choicesjs"],
            }

which is btw a much cleaner way than

            "dependencies": [
              "choicesjs#style",
              "choicesjs#script"
            ],

which does basically the same but only for presets right?

supporting this syntax for all types of assets would reduce code to maintain and allow flexibility or I'm wrong?

I mean if you run a check if (is_array($dependency[0])) { foreach($d[0] as $type=>$dep) { include } } doesn't seems to be much more complicated then parsing the string for #style and #script which does the same in the end right and supporting the old syntax for b/c might not be the biggest overhead.
ymmv

avatar Fedik
Fedik - comment - 20 Jan 2024

I not sure that I understood you correctly :)

we will get questions when do I use what and why shouldn't

It is simple: you always use dependencies, when your asset depend from asset of another type then use crossDependencies for this type of asset (it will be around 0.1-1% of all use).

In the core we have only a few asset that actualy use it.

It is more like an addittion, and not a replacement.
I would not want to re-define how the main dependecies is working. I think it already settled well, I mean everyone who use asset manager already get used to it.

Your sugestion sounds like we need to re-define how the dependecies is working. And we will be need to introduce some deprecations and have a mixed code/logic for the time of transition. I am not fan of it.

avatar Fedik Fedik - change - 20 Jan 2024
The description was changed
avatar Fedik Fedik - edited - 20 Jan 2024
avatar Fedik
Fedik - comment - 20 Jan 2024

I have added "example" in to description

avatar Fedik Fedik - change - 21 Jan 2024
The description was changed
avatar Fedik Fedik - edited - 21 Jan 2024
avatar Fedik Fedik - change - 26 Jan 2024
The description was changed
avatar Fedik Fedik - edited - 26 Jan 2024
avatar Fedik Fedik - change - 26 Jan 2024
The description was changed
avatar Fedik Fedik - edited - 26 Jan 2024
avatar Fedik Fedik - change - 17 Feb 2024
Labels Added: Feature PR-5.1-dev
avatar Fedik Fedik - change - 17 Feb 2024
The description was changed
avatar Fedik Fedik - edited - 17 Feb 2024
avatar Fedik
Fedik - comment - 21 Feb 2024

Okay, I found a way of doinng "dependecies": ["potato#script", "potato#style"].
Check in alternative branch 5.1-dev...Fedik:webasset-crossdependency2

But I do not very like that.
It introduces an extra parsing in to WebAssetItem class.
And it breaks the return type for preset, example call $preset->getDependecies() for preset:

{
  "name": "foobar",
  "type": "preset",
  "dependecies":["foo", "foo#style", "foo#script"]
}

Currently :

$preset->getDependecies() // will return ["foo", "foo#style", "foo#script"]

After requested changes:

$preset->getDependecies() // will return ["foo"]
// To get rest
$preset->getCrossDependecies() //will return [style => ["foo"], script => "foo"]

That will break code when someone use this preset outside of AssetManager.

I think use of extra property allow us to introduce this feature without extra hassle.
And migrate presets from current state to:

{
  "name": "foobar",
  "type": "preset",
  "dependecies":["foo"],
  "crossDependecies":["foo#style", "foo#script"]
}

(It also will be a b.c., but more controllable, because it wil affect only presets edited in json, instead of every existing preset)

What is your thought?

avatar Fedik Fedik - change - 19 Mar 2024
Title
[5.1] WebAsset cross-dependencies support
[5.x] WebAsset cross-dependencies support
avatar Fedik Fedik - edited - 19 Mar 2024
avatar HLeithner HLeithner - change - 24 Apr 2024
Title
[5.x] WebAsset cross-dependencies support
[5.2] WebAsset cross-dependencies support
avatar HLeithner HLeithner - edited - 24 Apr 2024
avatar Fedik Fedik - change - 28 Apr 2024
Labels Added: PR-5.2-dev
Removed: PR-5.1-dev
avatar HLeithner
HLeithner - comment - 2 Sep 2024

This pull request has been automatically rebased to 5.3-dev.

avatar HLeithner HLeithner - change - 2 Sep 2024
Title
[5.2] WebAsset cross-dependencies support
[5.3] WebAsset cross-dependencies support
avatar HLeithner HLeithner - edited - 2 Sep 2024

Add a Comment

Login with GitHub to post a comment