Skip to content

Comments

[4.0] WebAsset: redesign#25775

Merged
wilsonge merged 43 commits intojoomla:4.0-devfrom
Fedik:asset-4
Jan 5, 2020
Merged

[4.0] WebAsset: redesign#25775
wilsonge merged 43 commits intojoomla:4.0-devfrom
Fedik:asset-4

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Aug 3, 2019

Summary of Changes

As result of discussion in #25309 I have decided to make some changes.
The changes compared to existing WebAsset logic:

  • Now the assets are managed separately per type: script, style etc. (possible to add webcomponents also)
  • WebAssetItem represent a single file (instead of Collection of js, css, as before). This also change JSON schema, and simplify attributes editing (@wilsonge I will do separated pull later)
  • The Collection of js, css now represents by "empty" WebAssetItem with "cross-dependency" (called 'preset')
  • An "attach" method was removed, now it part of rendering process
  • AssetManager now allow to "register" an asset file as AssetItem, eg registerScript($asset)/registerStyle($asset)/registerAsset('foobar', $asset)

Questions that need to discuss:

Asset rendering and backward compatibility.
In current implementation, I have put all assets to _scripts/_stylesheet property of document (before rendering).
This allow to have all scripts/styles onBeforeCompileHead , but this also has a drawback.
It not allow to use AssetManager while this event (see #24858 and #24848).

So I think maybe we can break here a bit?
I mean, do not merge Asset files with _scripts/_stylesheet, but keep them in AssetManager.
Potential b.c. here that _scripts/_stylesheet will not contain files from active Asset at onBeforeCompileHead. (it also allow to drop WebAssetBeforeAttachEvent as useless)
Thoughts?

(I have split them)

Inline scrips/styles
I have skipped it for now, this part require full drop of _script/_style and move to AssetManager.
But because now an assets are managed separately (per type) Inline scrips/styles should be more easy to implement it in future.
In theory it is possible just ignore these properties, and proxy add[Script/Style]Declaration, but I am not sure currently how it will affect 3d extensions.
Thoughts?

(This is for another pull)

Other questions:
Make webcomponents to use AssetManager (no b.c.) Yes/No? (see #27268)
Proxy scriptOptions to AssetManager (minimum b.c.) Yes/No? (can be in another pull)

Testing Instructions

Apply patch run npm install and make sure everything still work as before

Methods comparison

For whom interesting

AssetManager WP
$wa->register[Script,Style,Foobar]() wp_register_[script,style]()
$wa->use[Script,Style,Foobar]()
$wa->registerAndUse[Script,Style,Foobar]()
wp_enqueue_[script,style]()
$wa->disable[Script,Style,Foobar]() wp_dequeue_[script,style]()
n.a. wp_add_inline_[script,style]()

Documentation Changes Required

yeap

@wilsonge @mbabker please review

For references:

doc by @mbabker Joomla Document/WebAsset Analysis
previous pulls #23463 #22435

Squashed commit of the following:

commit 306b785
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 20:38:36 2019 +0300

    deprecated a bit

commit 59dfcd7
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 20:16:14 2019 +0300

    webcomponent and webasset

commit f76df9f
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 19:28:02 2019 +0300

    interfaces

commit 9c0d929
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 18:46:57 2019 +0300

    clean up around

commit 16256fc
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 18:24:17 2019 +0300

    Drop $type property of AssetItem

commit b741f1c
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 17:40:17 2019 +0300

    more replaces of HTMLHelper to registerStyle

commit 21dd619
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 17:20:43 2019 +0300

    FormValidateAssetItem

commit dfc7727
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 17:12:24 2019 +0300

    cms/html use registerScript

commit d7f051e
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 16:29:05 2019 +0300

    Check for AttachBehavior, fix cross-dependency, remove Dispatcher

commit b22e05b
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 15:29:06 2019 +0300

    Revert document changes

commit c605ec7
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 15:22:32 2019 +0300

    Revert scripts/styles renderers to use previous rendering

commit fc3b9a8
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 15:16:11 2019 +0300

    Merge assets with an existing, to keep b.c.

commit 91e283c
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 14:56:06 2019 +0300

    flexible registerAsset()

commit 6c00787
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 14:27:26 2019 +0300

    enable to use, group to preset

commit 62ce690
Merge: c79d2c0 511547a
Author: Fedik <getthesite@gmail.com>
Date:   Sat Aug 3 12:57:07 2019 +0300

    Sync. Merge branch '4.0-dev' into asset3

commit c79d2c0
Author: Fedik <getthesite@gmail.com>
Date:   Thu Aug 1 20:34:40 2019 +0300

    help with debuging, fix of fallback

commit db921a6
Author: Fedik <getthesite@gmail.com>
Date:   Thu Aug 1 19:50:33 2019 +0300

    couple fixes

commit 3904911
Author: Fedik <getthesite@gmail.com>
Date:   Tue Jul 30 22:05:10 2019 +0300

    quick proxy for addScript/Style

commit 496baf1
Author: Fedik <getthesite@gmail.com>
Date:   Tue Jul 30 21:42:08 2019 +0300

    make something working

commit 36b730e
Author: Fedik <getthesite@gmail.com>
Date:   Tue Jul 30 20:50:45 2019 +0300

    update settings.json and provideAssets generation

commit 45871ae
Author: Fedik <getthesite@gmail.com>
Date:   Tue Jul 30 20:12:18 2019 +0300

    update settings.json

commit e863a2a
Author: Fedik <getthesite@gmail.com>
Date:   Tue Jul 30 19:25:37 2019 +0300

    use style

commit 80f7147
Author: Fedik <getthesite@gmail.com>
Date:   Tue Jul 30 18:46:33 2019 +0300

    update asset.json file

commit 18be7d2
Author: Fedik <getthesite@gmail.com>
Date:   Mon Jul 29 21:28:33 2019 +0300

    add lock, update registry file

commit ac6c13a
Author: Fedik <getthesite@gmail.com>
Date:   Mon Jul 29 19:59:19 2019 +0300

    fix enableDependencies() per type

commit 4e26280
Author: Fedik <getthesite@gmail.com>
Date:   Mon Jul 29 19:41:01 2019 +0300

    fix sorting

commit 78e1026
Author: Fedik <getthesite@gmail.com>
Date:   Mon Jul 29 19:12:59 2019 +0300

    EenableDependencies per type

commit 8cef0a5
Author: Fedik <getthesite@gmail.com>
Date:   Sun Jul 28 19:09:36 2019 +0300

    update WebAssetManager

commit 50fdc42
Author: Fedik <getthesite@gmail.com>
Date:   Sun Jul 28 16:18:13 2019 +0300

    update WebAssetItem

commit 897c382
Author: Fedik <getthesite@gmail.com>
Date:   Sun Jul 28 14:42:12 2019 +0300

    update schema
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Aug 3, 2019
@infograf768
Copy link
Member

This solves the issue I had with specific xx-XX.css admin lang which is now loaded after bootstrap and template css.

	<link href="/installmulti/joomla40/media/system/css/fields/switcher.min.css?a1bacb193a9e68d6496252f11a1d20e4" rel="stylesheet" />
	<link href="/installmulti/joomla40/administrator/templates/atum/css/bootstrap.min.css?a1bacb193a9e68d6496252f11a1d20e4" rel="stylesheet" />
	<link href="/installmulti/joomla40/administrator/templates/atum/css/template-rtl.min.css?a1bacb193a9e68d6496252f11a1d20e4" rel="stylesheet" />
	<link href="/installmulti/joomla40/administrator/language/fa-IR/fa-IR.css?a1bacb193a9e68d6496252f11a1d20e4" rel="stylesheet" />
	<link href="/installmulti/joomla40/administrator/templates/atum/css/fontawesome.min.css?a1bacb193a9e68d6496252f11a1d20e4" rel="stylesheet" />
etc.

@infograf768
Copy link
Member

@Fedik
Shall I set this to tested OK as I just tested the css?

@Fedik
Copy link
Member Author

Fedik commented Aug 15, 2019

@infograf768 yes if other part of the site still works good for you 😉
but the whole patch need to be reviewed by @wilsonge and others

@Fedik
Copy link
Member Author

Fedik commented Aug 15, 2019

well, I have to fix a conflict first

@wilsonge
Copy link
Contributor

I'm planning on looking at this and I'm aware of it - but I'm really busy with gsoc stuff as it ends this week. 20-30th I'm on holiday so I'm unlikely to be able to properly look at it until after I'm back from holiday. So please don't close this - and sorry it's taking me time to review it properly!

Fedik added 6 commits August 20, 2019 14:59
 Conflicts:
	administrator/templates/atum/index.php
	administrator/templates/atum/joomla.asset.json
	layouts/joomla/form/field/accesslevel-fancy-select.php
 Conflicts:
	libraries/src/Event/WebAsset/WebAssetBeforeAttachEvent.php
	libraries/src/WebAsset/WebAssetManager.php
	libraries/src/WebAsset/WebAssetRegistry.php
Fedik and others added 4 commits December 23, 2019 11:50
Co-Authored-By: SharkyKZ <sharkykz@gmail.com>
Co-Authored-By: SharkyKZ <sharkykz@gmail.com>
Co-Authored-By: SharkyKZ <sharkykz@gmail.com>
Co-Authored-By: SharkyKZ <sharkykz@gmail.com>
@SharkyKZ
Copy link
Contributor

With PR jquery-noconflict is not loading.

@Fedik
Copy link
Member Author

Fedik commented Dec 30, 2019

It does not required, but you can still load it if you need it

@jwaisner
Copy link
Member

jwaisner commented Jan 4, 2020

I have tested this item ✅ successfully on 9de1195


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25775.

@wilsonge wilsonge merged commit 7fa8d42 into joomla:4.0-dev Jan 5, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Jan 5, 2020

I have to say with this I'm slightly concerned that we are starting to make things much more complicated in the JSON structure. But at the same time as we're solving concrete problems I'm not sure there's much we can do about it. Can we please get the documentation and the JSON Schema updated please for this new approach

Thankyou for taking the time to investigating michael's review. It's very much appreciated by me :)

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 5, 2020
@Fedik
Copy link
Member Author

Fedik commented Jan 5, 2020

Schema update are there joomla/schemas#7
But do not merge for now please. I have to update a bit, about webcomponents ( to cover #27268)

I'm slightly concerned that we are starting to make things much more complicated in the JSON structure

yeah, me to, a bit :)
But it not that much complicated as much more content it contain now, because now an asset can contain only one file.
Also, because of this, I have introduced a "preset", a kind of collection that can "hold" a multiple different asset. It more like a "helper".
will see how it will work

the doc page I will look

btw, if extension has 1-2 asset files, it fine just use
$wa->registerAndUseScript('foobar.fancystuff', 'com_foobar/fancy-stuff.js'); instead of JSON.
JSON good for huge amount of assets, as we have in Joomla.

@roland-d
Copy link
Contributor

roland-d commented Feb 3, 2020

@Fedik Did you write up the documentation for the JSON format?

@Fedik
Copy link
Member Author

Fedik commented Feb 3, 2020

Documentation I have updated, it is there https://docs.joomla.org/J4.x:Web_Assets
And JSON schema are there https://github.com/joomla/schemas/blob/master/json-schema/web_assets.json (with properties description)

In documentation there just some examples of JSON, not sure what to write about schema.

@roland-d
Copy link
Contributor

roland-d commented Feb 3, 2020

@Fedik Thank you, I could not find the article on the docs.

@Fedik
Copy link
Member Author

Fedik commented Feb 3, 2020

btw, here is the "public link" to schema https://developer.joomla.org/schemas/json-schema/web_assets.json

@roland-d
Copy link
Contributor

roland-d commented Feb 3, 2020

@Fedik The only thing that is unclear to me is this:

"dependencies": [
        "core",
		"bootstrap"
      ],

How do I know which dependencies are available? The bootstrap is giving a fatal error now. I have no idea where to check what it is looking for. This might be something we can explain in the docs as well.

@Fedik
Copy link
Member Author

Fedik commented Feb 3, 2020

It some how this section https://docs.joomla.org/J4.x:Web_Assets#Register_an_asset
bootstrap actually part of vendors json media/vendor/joomla.asset.json

It should be available on on first access to WebAssetRegistry

But you have to check that file to get a correct name, there a couple of bootstrap variations, eg: bootstrap.css (styles), bootstrap.css.grid (another style), bootstrap.js (script), bootstrap.js.bundle (another script)

I had an idea to extend a debug plugin to print out all available assets, but this for some time later maybe, maybe for 4.2 or so 😃

@Fedik
Copy link
Member Author

Fedik commented Feb 3, 2020

and the core asset is part of system json media/system/joomla.asset.json

brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Feb 4, 2020
@roland-d
Copy link
Contributor

roland-d commented Feb 4, 2020

Hey @Fedik Thank you for the feedback so far.

It should be available on on first access to WebAssetRegistry

That doesn't seem to be the case. When I inspect $wa in the index.php of the template, it has some json files loaded but nothing parsed.
image

When I add core as dependency in the templates/cassiopeia/joomla.asset.json file like this:

"name": "template.cassiopeia.ltr",
      "type": "preset",
      "dependencies": [
		"core",
        "template.cassiopeia.ltr#style",
        "template.cassiopeia#script"
      ]

It also bails with an error

Error: Unsatisfied dependency "core" for an asset "template.cassiopeia.ltr" of type "preset": Unsatisfied dependency "core" for an asset "template.cassiopeia.ltr" of type "preset"

It looks like, the json files need to be parsed first before I can load any preset/asset with a dependency. Any ideas about this?

@Fedik
Copy link
Member Author

Fedik commented Feb 4, 2020

It should be available on on first access to WebAssetRegistry
When I inspect $wa in the index.php of the template, it has some json files loaded but nothing parsed.

that correct, they will be parsed on "first access", mean on first $registry->get() call, that will be performed on any $wa->useScript('foobar') call

When I add core as dependency ... It also fails with an error.

That also correct, see what type of asset do you use. If your asset is a script then all dependency also must be script, if your asset are style then all dependency also must be type of style and so on, preset for presets.
There no support of "cross type" dependency (except for presets).

Your example:

"name": "template.cassiopeia.ltr",
"type": "preset", <----- Asset type
"dependencies": [
  "core",   <------ Looking for another "preset" with name "core", not for "script" asset
  "template.cassiopeia.ltr#style",
  "template.cassiopeia#script"
]

As error say, there no "preset" with name "core".
Here is about presets https://docs.joomla.org/J4.x:Web_Assets#Working_with_a_presets with explanation what is special in presets.

Correct would be:

"name": "template.cassiopeia.ltr",
"type": "preset",
"dependencies": [
  "core#script",
  "template.cassiopeia.ltr#style",
  "template.cassiopeia#script"
]

But again, this only for "preset" type, other asset types does not support "cross type" dependency.

@roland-d
Copy link
Contributor

roland-d commented Feb 4, 2020

@Fedik The exteneded explanation is much appreciated. This gives me a lot of insight and it is working now.

@Fedik
Copy link
Member Author

Fedik commented Feb 4, 2020

It was a bit different in first implementation,
In first implementation an asset hold multiple different files css, js, and then after some discussions I have change to handle 1 asset = 1 file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.