Skip to content

Comments

[4.0] WebAsset support for inline assets#27756

Merged
wilsonge merged 2 commits intojoomla:4.0-devfrom
Fedik:asset-inline2
Feb 6, 2020
Merged

[4.0] WebAsset support for inline assets#27756
wilsonge merged 2 commits intojoomla:4.0-devfrom
Fedik:asset-inline2

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Feb 1, 2020

This is for make the thing complete

Summary of Changes

The patch add support of inline content to asset manager with support of optional before/after positions (discussion here #25309). And improves script/style rendering a bit.
And a new headache surprise for @wilsonge 😃
Also the script options rendered as inline asset with position "before" a core asset.

New methods of asset manager: addInlineStyle, addInlineScript.
Simple example:

$wa->addInlineScript('alert("I am alert!");');

Example with support of positioning:

$wa->addInlineScript('alert("I am alert after jQuery!");', 
  ['position' => 'after'], [], ['jquery']);

$wa->addInlineScript('alert("I am alert before jQuery!");', 
  ['position' => 'before'], [], ['jquery']);

Testing Instructions

Apply patch, and try play with new methods.
Example add this to index.php of Atum template:

$this->getWebAssetManager()
	->addInlineScript('console.log("after core")', ['position' => 'after'], ['type' => 'module'], ['core'])
	->addInlineScript('console.log("regular1")', [], [], ['core'])
	->addInlineScript('console.log("regular2")', [], [], ['core'])
	->addInlineStyle('#content .card-header{background:green;color:white;}')
	->addInlineStyle('/* random style AFTER fontawesome */', ['position' => 'after'], [], ['fontawesome'])
	->addInlineStyle('/* random style BEFORE fontawesome */', ['position' => 'before'], [], ['fontawesome'])
;

Then check the source code of the page, and make sure the content are rendered and have a requested positions.

Merging the patch, notes

In theory the patch can be merged as is now, it does not introduce b.c.

However, we can pretty safely proxy existing $doc->addScriptDeclaration()/$doc->addStyleDeclaration() to $doc->assetmanager->addInline(). In this case $doc->_style/$doc->_script will stay empty (ignored), wich introduce a little b.c.
But we already made b.c. changes for these properties (to support array instead of string #25357), I think it acceptable.
What do you think @wilsonge @mbabker . If you agree I will do these changes, otherwise I leave as is.

Documentation Changes Required

yeap

for reference #25309 #23463 #22435

@dgrammatiko
Copy link
Contributor

@Fedik is the output serialised (eg inline scripts not appended after the linked ones)?
I tried to do it in the old code #14240

@Fedik
Copy link
Member Author

Fedik commented Feb 2, 2020

No and Yes.
By default it doing "old way", render all "files.js" elements and then inline elements. Eg:

script1.js
script2.js
script3.js
content of inline1
content of inline2
content of inline3

if you specify the position and dependency, then it rendered at requested position:

$wa
  ->addInlineScript('content of inline3', ['position' => 'before'], [], ['script1'])
  ->addInlineScript('content of inline4', ['position' => 'after'], [], ['script1']);

Will produce:

content of inline3
script1.js
content of inline4
script2.js
script3.js
content of inline1
content of inline2
content of inline3

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 2, 2020

@Fedik FWIW since this is a brand new functionality please drop this: By default it doing "old way", render all "files.js" elements and then inline elements. as it's completely wrong. Explanation #14240 (comment)

@Fedik
Copy link
Member Author

Fedik commented Feb 2, 2020

If you need "this specific inline CSS", to be loaded after "that specific CSS file", you explicitly set dependency and position of your inline CSS (before/after). Instead of playing dice.
Look at my previous example.

However there should not be a big problem to change it (it just about how to render).
For me it fine as it is.

@wilsonge wilsonge merged commit 1766094 into joomla:4.0-dev Feb 6, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Feb 6, 2020

Le documentation please. This seems pretty b/c and incorporates the last parts of the JDocument API. I'm happy with this

@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 6, 2020
@Fedik Fedik deleted the asset-inline2 branch February 6, 2020 21:53
@Fedik
Copy link
Member Author

Fedik commented Feb 8, 2020

@wilsonge please review https://docs.joomla.org/J4.x:Web_Assets
I have added couple new sections, about inline and about asset stages

@Fedik
Copy link
Member Author

Fedik commented Feb 8, 2020

btw, what do you think do we need to move addScriptOptions() to assetManager (proxy them) or leave it for j5?

@wilsonge
Copy link
Contributor

I'd leave them out totally I think? Script options aren't really related to the assets in my view. I mean they do support the asset. But they don't depend on the asset existing. Unless you're planning on having proper error page generated by php whenever a script option doesn't exist for an asset or something?

@Fedik
Copy link
Member Author

Fedik commented Feb 21, 2020

Script options aren't really related to the assets in my view

But also the script options makes no sense without scripts 😉

Unless you're planning on having proper error page generated by php whenever a script option

No, nothing like that, the options is not required anyway, the script should have fallback to some default value, as usual.

okay, I leave it as is for now, nothing really important

@dgrammatiko
Copy link
Contributor

@Fedik quick question: are the assets using their own version or you're still using the global mediaVersion from the document?

@wilsonge
Copy link
Contributor

But also the script options makes no sense without scripts 😉

True but my gut is about them not being required like you say. I mean I'm not totally convinced by my own arguments 🤷‍♂

@Fedik
Copy link
Member Author

Fedik commented Feb 21, 2020

are the assets using their own version or you're still using the global mediaVersion from the document?

if asset have defined version property then it will be used, otherwise it use "global from document"

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants