@diosmosis opened this Pull Request on February 8th 2022 Member

Description:

cc @tsteur

Now that there are multiple plugins that have Vue code its possible to experiment with different ways of loading the JS, instead of loading everything into a single file and loading it (which might be a problem since the Vue templates get compiled). (This is the simplest/quickest approach I could think of.)

This PoC provides two different methods of loading the UMD JS files:

  • Load each plugin UMD file by itself asynchronously. This results in one <script defer> element per plugin with Vue code. Which could be a lot of AJAX requests eventually.
  • Have a set limit of chunks that can be loaded asynchronously and divide the plugin modules among them so the number of AJAX requests is limited.

In both they are not loaded into the main assetmanager JS files, they are instead put into separate JS files and loaded with the defer <script> attribute. So they will all load asynchronously and run in the correct order after.

@tsteur let me know what you think. I'm not sure if there will need to be some performance testing on cloud just to see if it's worth it.

Changes:

  • Create PluginUmdAssetFetcher.php to group assets into chunks. Used by AssetManager.
  • Modify compute command to output size of chunks as well.
  • Add two undocumented INI config to control chunking: [General] assets_umd_load_individually, [General] assets_umd_chunk_count

Review

@tsteur commented on February 9th 2022 Member

👍 I guess it'll be worth it for plugins like TagManager etc where the files are only needed in specific screens. It be worth giving this a try and we can always adjust if needed 👍 @sgiehl maybe you can have a look at this?

@diosmosis commented on February 9th 2022 Member

@tsteur note this still loads all javascript beforehand, it just does it in N simultaneous ajax requests rather than one big request. It doesn't load the files right before they are needed, that's a bit harder to do.

@tsteur commented on February 9th 2022 Member

got it 👍

One thing we will need to test once this is merged and a beta is out that it also works well with Matomo for WordPress and plugins. Or maybe we can also test it as part of this PR. It would require a non-core plugin though and we would need to define:

To locate plugins from a custom directory by setting an environment variable MATOMO_PLUGIN_DIRS or a $GLOBALS['MATOMO_PLUGIN_DIRS'] variable in $MATOMO_ROOT/bootstrap.php.

Like we have say also plugins in $matomoDir/my-custom-plugins/ (eg `$matomoDir/my-custom-plugins/my-custom-plugin) and then it would also need to load the file correctly from that directory.

Some people use this to have non-core plugins with different file permissions in a different directory and Matomo for WordPress also uses it. I'm saying this as the JS files will need to be loaded from a different path as they won't be in the /plugins/ directory. Not sure it's relevant or not.

@diosmosis commented on February 9th 2022 Member

Some people use this to have non-core plugins with different file permissions in a different directory and Matomo for WordPress also uses it. I'm saying this as the JS files will need to be loaded from a different path as they won't be in the /plugins/ directory. Not sure it's relevant or not.

Yes, that's relevant, currently /plugins/ is hardcoded. I'll change the PR to use Plugin\Manager::getPluginsDirectories(), hopefully that's enough.

@github-actions[bot] commented on February 18th 2022 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@sgiehl commented on February 22nd 2022 Member

@diosmosis It seems the cachebuster is currently not appended to the additional js requests. Guess this part needs to be adjusted so the additional defer doesn't prevent adding the cachebuster:
https://github.com/matomo-org/matomo/blob/4764df9b0c795301976788df87fe314ecc7acece/core/View.php#L360-L378

@diosmosis commented on February 23rd 2022 Member

@sgiehl updated

@diosmosis commented on February 23rd 2022 Member

Oh looks like tests aren't passing, not sure why disable_merged_assets is enabled for me no matter what... looking into it, no need to review right now.

@diosmosis commented on February 23rd 2022 Member

@sgiehl ok should be good for another review

@diosmosis commented on February 23rd 2022 Member

Looks good to me now. We could though think about adding some tests for the new asset loading stuff.

Sure I'll add some tests.

@diosmosis can you think of any potential problem that might cause? Just wondering if we shall merge that now, before an upcoming release or better wait after it.

Can't think of any, but sure it can wait. It'll take time to add some tests anyway.

@diosmosis commented on March 2nd 2022 Member

@sgiehl added tests and fixed some issues. There's one image that needs to be updated, I'll update that before merging. Pinging in case you want to review the rest of the code first.

This Pull Request was closed on March 4th 2022
Powered by GitHub Issue Mirror