[Vue] Chunk UMD JavaScript into a set of asynchronously loaded files #18761

diosmosis · 2022-02-08T23:15:06Z

Description:

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

… are loaded asynchronously

tsteur · 2022-02-09T02:19:01Z

👍 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 · 2022-02-09T11:24:27Z

@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 · 2022-02-09T18:52:12Z

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 · 2022-02-09T20:37:41Z

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.

…d config) and get to work

…asset-size

github-actions · 2022-02-18T01:50:47Z

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

peterhashair

Looks good to me, Test Couple plugins, works as expected, it reduce the head script size by a lot. Worse case, use this config will reset to original disable_merged_assets = 1. Ping @sgiehl

sgiehl · 2022-02-22T16:10:26Z

@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:

matomo/core/View.php

Lines 360 to 378 in 4764df9

    
           $tagJs  = 'cb=' . $cacheBuster->piwikVersionBasedCacheBuster(); 
        
           $tagCss = 'cb=' . $cssCacheBusterId; 
        
           $pattern = array( 
        
               '~<script type=[\'"]text/javascript[\'"] src=[\'"]([^\'"]+)[\'"]>~', 
        
               '~<script src=[\'"]([^\'"]+)[\'"] type=[\'"]text/javascript[\'"]>~', 
        
               '~<link rel=[\'"]stylesheet[\'"] type=[\'"]text/css[\'"] href=[\'"]([^\'"]+)[\'"] ?/?>~', 
        
               // removes the double ?cb= tag 
        
               '~(src|href)=\"index.php\?module=([A-Za-z0-9_]+)&action=([A-Za-z0-9_]+)\?cb=~', 
        
           ); 
        
           $replace = array( 
        
               '<script type="text/javascript" src="$1?' . $tagJs . '">', 
        
               '<script type="text/javascript" src="$1?' . $tagJs . '">', 
        
               '<link rel="stylesheet" type="text/css" href="$1?' . $tagCss . '" />', 
        
               '$1="index.php?module=$2&amp;action=$3&amp;cb=', 
        
           ); 
        
           return preg_replace($pattern, $replace, $output);

diosmosis · 2022-02-23T00:14:49Z

@sgiehl updated

…g some tests)

diosmosis · 2022-02-23T01:57:23Z

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.

…r relative path will work

diosmosis · 2022-02-23T03:14:50Z

@sgiehl ok should be good for another review

sgiehl

Looks good to me now. We could though think about adding some tests for the new asset loading stuff.
@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.

diosmosis · 2022-02-23T17:40:14Z

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 · 2022-03-02T15:51:42Z

@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.

sgiehl

LGTM

proof of concept for chunking UMD JavaScript into a set of files that…

8f7ca0d

… are loaded asynchronously

diosmosis added 4 commits February 10, 2022 09:30

take into account alternative plugin directories

5fa745a

make chunk count and load umds individually configurable (undocumente…

04332d9

…d config) and get to work

fix a bug and add chunk JS sizes to output of development:compute-js-…

6dcdd1b

…asset-size

document

7509fdb

diosmosis added this to the 4.8.0 milestone Feb 10, 2022

diosmosis added the Needs Review PRs that need a code review label Feb 10, 2022

diosmosis changed the title ~~[Vue] PoC for chunking UMD JavaScript into a set of asynchronously loaded files that~~ [Vue] Chunk UMD JavaScript into a set of asynchronously loaded files Feb 10, 2022

fill out TODO documentation

2b5de92

github-actions bot added the Stale The label used by the Close Stale Issues action label Feb 18, 2022

diosmosis added Do not close PRs with this label won't be marked as stale by the Close Stale Issues action and removed Stale The label used by the Close Stale Issues action labels Feb 18, 2022

diosmosis requested a review from justinvelluppillai February 21, 2022 21:41

peterhashair approved these changes Feb 22, 2022

View reviewed changes

Merge branch '4.x-dev' into vue-asset-chunking

88d1a8f

make sure cache buster is added to chunk script srcs

acb5247

add some checks in case a chunk does not exist on disk (happens durin…

c096315

…g some tests)

use realpath on test PIWIK_INCLUDE_PATH so search/replace on paths fo…

5448b81

…r relative path will work

sgiehl approved these changes Feb 23, 2022

View reviewed changes

sgiehl removed the Needs Review PRs that need a code review label Feb 23, 2022

justinvelluppillai modified the milestones: 4.8.0, 4.9.0 Mar 1, 2022

diosmosis added 5 commits March 1, 2022 08:21

add integration test and get to pass

945be50

Merge branch '4.x-dev' into vue-asset-chunking

57c8d12

fix for when disable_merged_assets=1

f9dd323

fix condition

7c57f84

fix ui test failure

022311e

sgiehl approved these changes Mar 4, 2022

View reviewed changes

update screenshot

6924ad5

diosmosis merged commit b774954 into 4.x-dev Mar 4, 2022

diosmosis deleted the vue-asset-chunking branch March 4, 2022 16:35

diosmosis mentioned this pull request Mar 4, 2022

[Vue] Fix regression in plugin order determination, new param keepUnresolved is not used in function. #18886

Merged

11 tasks

This was referenced Mar 7, 2022

Fix comparison trends might be displayed incorrect for certain languages #18832

Merged

[Vue] make sure chunk assets can be removed #18929

Merged

justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vue] Chunk UMD JavaScript into a set of asynchronously loaded files #18761

[Vue] Chunk UMD JavaScript into a set of asynchronously loaded files #18761

diosmosis commented Feb 8, 2022 •

edited

tsteur commented Feb 9, 2022

diosmosis commented Feb 9, 2022

tsteur commented Feb 9, 2022

diosmosis commented Feb 9, 2022

github-actions bot commented Feb 18, 2022

peterhashair left a comment •

edited

sgiehl commented Feb 22, 2022

diosmosis commented Feb 23, 2022

diosmosis commented Feb 23, 2022

diosmosis commented Feb 23, 2022

sgiehl left a comment

diosmosis commented Feb 23, 2022

diosmosis commented Mar 2, 2022

sgiehl left a comment

[Vue] Chunk UMD JavaScript into a set of asynchronously loaded files #18761

[Vue] Chunk UMD JavaScript into a set of asynchronously loaded files #18761

Conversation

diosmosis commented Feb 8, 2022 • edited

Description:

Review

tsteur commented Feb 9, 2022

diosmosis commented Feb 9, 2022

tsteur commented Feb 9, 2022

diosmosis commented Feb 9, 2022

github-actions bot commented Feb 18, 2022

peterhashair left a comment • edited

Choose a reason for hiding this comment

sgiehl commented Feb 22, 2022

diosmosis commented Feb 23, 2022

diosmosis commented Feb 23, 2022

diosmosis commented Feb 23, 2022

sgiehl left a comment

Choose a reason for hiding this comment

diosmosis commented Feb 23, 2022

diosmosis commented Mar 2, 2022

sgiehl left a comment

Choose a reason for hiding this comment

diosmosis commented Feb 8, 2022 •

edited

peterhashair left a comment •

edited