Skip to content
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

[Vue] add support for on demand loading of plugin UMD files when a component in a plugin is first used. #18980

Merged
merged 43 commits into from Aug 31, 2022

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Mar 22, 2022

Description:

This PR provides on demand loading of a plugin's Vue module, so it does not have to be loaded on each page load.

To set a plugin's Vue module to be loaded on demand you'd have to:

  • override Plugin::shouldLoadUmdOnDemand() and return true
  • if the plugin defines angularjs adapters, move them all to a separate file manually included via the AssetManager event and set the component property to a reference like { plugin: 'MyPluginName', component: 'MyComponentName' }.

If you need to use a plugin that's loaded like this in another plugin, you won't be able to import as you would normally. Instead you'll need to use the importPluginUmd method:

import { importPluginUmd } from 'CoreHome';

importPluginUmd('TagManager').then((module: typeof import('TagManager')) => {
  // ...
});

Note: that adapters for stores or anything that gets added via angular(...).adapter will not be available, so making a plugin load on demand is a potential BC break. For TagManager this is acceptable since the old models weren't meant to be reused.

To see a plugin that uses this, see: matomo-org/tag-manager#461

Review

@diosmosis diosmosis changed the title [Vue] Proof of concept for on demand loading of plugin UMD files when a component in a plugin is first used. [Vue] PoC for on demand loading of plugin UMD files when a component in a plugin is first used. Mar 22, 2022
@diosmosis
Copy link
Member Author

Adding this to the current milestone, but I'm not sure when it is desired to include it.

@diosmosis diosmosis modified the milestones: 4.9.0, 4.10.0 Mar 22, 2022
@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Jul 20, 2022
@sgiehl sgiehl removed Stale The label used by the Close Stale Issues action Stale for long The label used by the Close Stale Issues action labels Jul 21, 2022
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 29, 2022
@sgiehl sgiehl modified the milestones: 4.12.0, 5.0.0 Aug 22, 2022
@sgiehl sgiehl changed the base branch from 4.x-dev to 5.x-dev August 22, 2022 19:22
@sgiehl
Copy link
Member

sgiehl commented Aug 22, 2022

@diosmosis would you mind rebasing that one to 5.x-dev? We've decided not to put that into the last 4.x release to lower the risk of regressions we might need to fix. Also we will have more time to test that, after it was merge to 5.x-dev...

@diosmosis
Copy link
Member Author

@sgiehl should be up to date w/ 5.x-dev

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Aug 23, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it with the TagManager PR and seems to work smoothly. Left a comment. Otherwise I guess this should be good to merge.

plugins/CoreHome/vue/src/index.ts Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Aug 26, 2022
@diosmosis diosmosis added the Needs Review PRs that need a code review label Aug 26, 2022
@diosmosis
Copy link
Member Author

fixed

@sgiehl
Copy link
Member

sgiehl commented Aug 29, 2022

@diosmosis Could you check the failing UI tests? Seems the language selection is no longer visible in installation and updater. Seems there is a javascript error in that case.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Aug 29, 2022
@diosmosis
Copy link
Member Author

@sgiehl fixed

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works now and tests are also passing again

@sgiehl sgiehl merged commit b73134b into 5.x-dev Aug 31, 2022
@sgiehl sgiehl deleted the vue-async-umd-loading branch August 31, 2022 07:54
bx80 pushed a commit that referenced this pull request Nov 25, 2022
…mponent in a plugin is first used. (#18980)

* Proof of concept for on demand loading of plugin UMD files when a component in a plugin is first used.

* add more comments

* debug ui test failure

* more debugging

* even more debugging

* try again?

* more debugging

* more debugging

* more fixes

* more debugging

* fix path

* more debugging

* fix path again, more debugging

* try this again

* try this again

* try this again

* another debug attempt

* more debugging

* another attempt

* more debugging

* more debugging

* more debugging

* more debugging

* more debugging

* more debugging

* more debugging

* more debugging

* try to fix issue

* undo debugging changes

* fix last fix

* add integration tests for new functionality

* revert changes to FrontController which should not have been added to PR

* Login always loaded

* put list of plugins to load on demand client side and modify check for plugin existence in vue-entry processing to use it

* remove unneeded interim event

* handle case where pluginsToLoadOnDemand is not set as global

Co-authored-by: sgiehl <stefan@matomo.org>
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.

None yet

4 participants