New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0][RFC] Elephant can manage assets also. The WebAssetRegistry class to manage assets and it`s dependacies #22435

Merged
merged 37 commits into from Oct 14, 2018

Conversation

Projects
None yet
9 participants
@Fedik
Contributor

Fedik commented Sep 30, 2018

Pull Request for Issue #12402, improved version of #8913 .

Summary of Changes

The patch introduce new class WebAssetRegistry to manage the assets, and it's dependencies.
The assets - > read as CSS/JS library.

How it works

All registry information stored in a registry file, a simple json file called joomla.asset.json.
I made an example for media/vendor/joomla.asset.json, media/system/joomla.asset.json, and for each template atum/joomla.asset.json, cassiopeia/joomla.asset.json.

The Registry file are for:

  • The registry file contain an information about provided assets.
  • Each asset provide dependencies, and js/css files which be attached to the Document on header rendering event.
  • Each asset can be overridden by another registry file (take a look in atum/joomla.asset.json where overridden bootstrap.css, and font-awesome).

To add your registry file:

Factory::getContainer()->get('webasset')
  ->addRegistryFile('media/blabla/path/joomla.asset.json')

How to use, enable/disable assets:

/** @var Joomla\CMS\WebAsset\WebAssetRegistry $wa */
$wa = Factory::getContainer()->get('webasset');

// Enable Core asset
$wa->enableAsset('core');
// If the asset do not exists, then Exception will be thrown

// Disable Bootstrap CSS
$wa->disableAsset('bootstrap.css'); 
// bootstrap.css - is an asset name, not a file name !!!
// look in media/vendor/joomla.asset.json to see why

NOTE: Method enableAsset() / disableAsset() do NOT add anything to the document. It just change the asset state. The files of all active assets will be attached to the Document on before header rendering.

By default WebAssetRegistry use a "lazy loading" of assets.
The registry file parsed only when WebAssetRegistry requested for new asset.
All dependencies will be resolved while attaching WebAssetRegistry to the document. See WebAssetRegistry::attach()

Also I have made an example in behavior.core, bootstrap.framework.

Further changes/ideas

Allow custom class per asset, that can handle stuff like in 'behavior.core':

function onBeforeAtach() {
  $doc->addScriptOptions('system.paths', [
	'root' => Uri::root(true),
	'rootFull' => Uri::root(),
	'base' => Uri::base(true),
  ]);
}

In theory, this allow to clean up JHtml from adding a scripts.

For now feedback are welcome! :neckbeard:

Documentation Changes Required

yes, if it will be accepted

ping @wilsonge @mbabker @dgrammatiko @C-Lodder @laoneo
also everyone else who doing frondend stuff

Fedik added some commits Sep 30, 2018

*
* @since __DEPLOY_VERSION__
*/
public function setState($state)

This comment has been minimized.

@mbabker

mbabker Sep 30, 2018

Member

Use scalar typehinting and return types, remove type cast in the method.

public function setState(int $state): self
{
    $this->state = $state;

    return $this;
}
@mbabker

This comment has been minimized.

Member

mbabker commented Sep 30, 2018

Personally, I'd rather all new events have their own Event subclasses versus everything being dispatched with the generic event class.

public function onWebAssetStateChanged(Event $event): void
{
    $asset = $event->getArgument('asset');
}

versus

public function onWebAssetStateChanged(WebAssetStateChangedEvent $event): void
{
    $asset = $event->getAsset();
}

You get a better self-documenting API versus having to find the triggering source code to see exactly what arguments are available.

@dgrammatiko

This comment has been minimized.

Contributor

dgrammatiko commented Sep 30, 2018

@Fedik nice to see that you're pushing this idea (I was always counting that you will come up with such a PR, thus I never actually deleted the assets lines in the build tools 😉 ).

There are some consideration that the production team should consider here:

  • Everything in the Javascript world is becoming a module, so I guess there should be a way to manage modules along the old es5 scripts. (small explanation here: if you have a <script type="module" src="blabla.js"> by default this script is deferred and scoped differently than a normal es6 script. This obviously imposes some challenges with jQuery scripts and the order of execution but it's another talk altogether).

  • I would expect J4 to be able to handle critical/lazy-loaded assets, at the end of the day these are the current best practices...

  • If you keep JHtml::script, JHtml::stylesheet, addScript, addStylesheet (also some bright minds are using the addCustomTag to inject css or js), then there will be way too many options to handle assets which obviously will make the dev life more complicated instead of simplifying things. So the question (this goes to the production team) is: are you willing to break the current status quo?

@Fedik

This comment has been minimized.

Contributor

Fedik commented Sep 30, 2018

in build/build-modules-js/settings.json
I assume they should all be minified - they're not

@brianteeman there a sources, they will be minified and moved to /media while run npm install.
btw, thanks for English corrections, I will fix it

@Fedik

This comment has been minimized.

Contributor

Fedik commented Sep 30, 2018

@dgrammatiko I think you a bit misunderstood the idea of having modules in JavaScript, or I misunderstood :neckbeard:

The module is like a library, which doing nothing itself, no interaction with outside world, it even static.
So use a module definition for regular scripts (which doing DOM manipulation etc.) is wrong.

Also in current state, the module cannot easily live in dynamic app like CMS,
because limitation of import. Even when dynamical import (http://2ality.com/2017/01/import-operator.html) will be supported by all browsers, it still a question, how it can work in context of CMS.

For my understanding CMS require a customizable loader, like require.js.

BUT, we still can use defer for regular scripts.
And I think for J4, we can do, force every script to be defer, and if there at least 1 inline script, we switch off the defer mode.
I think it already can be done on scripts rendering.
I use similar approach in my "optimization plugin", I force every script to defer, and if there are inline code exist then just disabled defer, works fine for couple years 😃

I would expect J4 to be able to handle critical/lazy-loaded assets, at the end of the day these are the current best practices

I heard about it, but I have no idea, how it can live here 😄

JHtml::script, JHtml::stylesheet, addScript, addStylesheet

Well, they do not conflict, they are complement each other. But there huge misunderstanding what they doing.
TBH, I have no idea what we can do here.

Technically in the asset you can define any path, so you can even copy whole /node_modules/ to your template, then define every node_module as asset in JSON file, and it will work.
So yes it removes a limitation that you must place the CSS file in /css folder to make JHtml::stylesheet override to work.

@mbabker

This comment has been minimized.

Member

mbabker commented Sep 30, 2018

JHtml::script, JHtml::stylesheet, addScript, addStylesheet

Well, they do not conflict, they are complement each other. But there huge misunderstanding what they doing.
TBH, I have no idea what we can do here.

Technically in the asset you can define any path, so you can even copy whole /node_modules/ to your template, then define every node_module as asset in JSON file, and it will work.
So yes it removes a limitation that you must place the CSS file in /css folder to make JHtml::stylesheet override to work.

The directory based override convention works great for end users who just need to copy a file and tweak something. If we move to a system where you have to modify manifest files or use plugins to re-register an asset's path that's adding a bit of complexity to what has previously been a rather simple system. So just keep that in mind.

foreach ($paths['stylesheet'] as $path => $attr)
{
unset($attr['__isExternal'], $attr['__pathOrigin']);
$doc->addStyleSheet($path, ['version' => 'auto'], $attr);

This comment has been minimized.

@mbabker

mbabker Sep 30, 2018

Member

Version needs to be a configurable attribute. Not everything should use the CMS' random hash for a query string version, there is no issue with using the version string of the libraries in use (and if you're really one of those paranoid people who have issues with a version string in a URL you're really going to hate some of the file headers on assets and you can deal with changing the query string on your own).

This comment has been minimized.

@Fedik

Fedik Sep 30, 2018

Contributor

I thought to use $asset->getVersion(), but then made auto, to be safe and easy,
will see

This comment has been minimized.

@mbabker

mbabker Sep 30, 2018

Member

I'd go for this.

If $asset->getVersion() has any non-empty value, use it (knowing full well "auto" is a special case inside the Document API).

Have an attribute on the registry to decide whether unversioned assets ($asset->getVersion === '') should be auto-versioned (the ['version' => 'auto'] bit that's here now) or left unversioned. Default it to true, let someone turn it off if they choose to.

@dgrammatiko

This comment has been minimized.

Contributor

dgrammatiko commented Oct 1, 2018

If we move to a system where you have to modify manifest files or use plugins to re-register an asset's path that's adding a bit of complexity to what has previously been a rather simple system

You should be able to do HTMLHelper::script or HTMLHelper::stylesheet just before adding the assets to the document->_scripts or _stylesheets. That will keep the same old functionality intact. But is it wise to have multiple methods for adding assets into a page?

@Fedik you're quite right modules are best fit for declarative scripts not imperative (most of the Dom things in Joomla). Also modules make sense if you are going to do some tree shaking and that requires node and that's also a dead end...
So from all this part of the modules comment just keep, as you already did, the part that every script tag needs to have a defer attribute
Also big thumbs up for require.js but I guess this idea won't anywhere...

@Fedik

This comment has been minimized.

Contributor

Fedik commented Oct 1, 2018

Also big thumbs up for require.js but I guess this idea won't anywhere...

maybe in someday, in the galaxy far far far far away ... 😄

... as you already did, the part that every script tag needs to have a defer attribute

I did not made it in this PR, but have the idea for separated PR, but just an idea for now 😉

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 1, 2018

The problem is not with require.js or any of that tooling. The problem is you are in an environment where there is an end user facing feature for runtime resolution of assets that enables end users to create overrides. You aren't working in a purely compiled environment, you aren't working in an environment where all the paths are going to be static.

If the JavaScript and browser tooling reach a point where the capabilities that Joomla has can be emulated in a way other than what JHtml does to resolve paths at runtime, that can be considered. Until then, talking down on the project because it doesn't push for a compiled build environment that only "real" developers can use does nothing but make people feel alienated.

@dgrammatiko

This comment has been minimized.

Contributor

dgrammatiko commented Oct 1, 2018

you aren't working in an environment where all the paths are going to be static

require.js can handle this in the same manner script tags are appended in the html.

talking down on the project because it doesn't push for a compiled build environment

You got me wrong, all I said was that you should try to ease the path for modules (by forcing defer) not actually implement it right now (yes till dynamic imports are supported everywhere there's no point even trying to do it)

@Fedik

This comment has been minimized.

Contributor

Fedik commented Oct 1, 2018

The problem is you are in an environment where there is an end user facing feature for runtime resolution of assets that enables end users to create overrides.

After some tweaks WebAssetRegistry allow to generate a config for require.js, which can be used as script/module loader.

but all this not in current PR

@Fedik

This comment has been minimized.

Contributor

Fedik commented Oct 2, 2018

@mbabker Here another example about what is doing enable/disable and add/remove methods:

// Empty asset registry
$wa = new WebAssetRegistry;

// Add some assets
$wa->addAsset(new WebAssetItem("🍻"));
$wa->addAsset(new WebAssetItem("🐘"));

// At this point we have 2 asset in registry, with STATE_INACTIVE
// So nothing will be added to Document
$wa->attachActiveAssetsToDocument($document); // Will do nothing, because all assets is inactive

// Enable asset
$wa->enableAsset("🐘");

// At this ponit we still have 2 assets, but only 1 is Active
$wa->attachActiveAssetsToDocument($document); // Will add files from "🐘" asset to Document

// Try to enable not-existing asset
$wa->enableAsset("🐛"); // Will throw Exception

And use of RegistryFile just doing addAsset() for you, all you need is just enable/disable existing.
Of course you still can do $wa->addAsset(new WebAssetItem("🍪"))->enableAsset("🍪"), if need.

@wilsonge wilsonge merged commit 7b3ca9b into joomla:4.0-dev Oct 14, 2018

3 of 4 checks passed

continuous-integration/drone/pr the build failed
Details
Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 14, 2018

Let's take this as a first pass. I'm pretty sure there's some improvements to be made here - but I'm happy with this as the base concept

@Fedik

This comment has been minimized.

Contributor

Fedik commented Oct 15, 2018

oh, that was fast 😄

for note, one issue left:

@mbabker : If I enable an asset with dependencies I expect the dependencies to be enabled as well.

This might be a case of trying to be too lazy. It's fine to defer parsing the JSON files until you actually start adding WebAssetItem entries to the internal store, but once you start populating WebAssetRegistry::$assets with objects it should always contain the full resource tree. It really shouldn't be a partial snapshot.

I think it is pretty important.
I will try to fix it, in one of next pull request.

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