-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
AngularLoader: Support 'settingsFactory' callbacks in angular modules. #18731
Conversation
(Standard links)
|
This is a good idea. The test looks sensible (though I added a suggestion above for more assertions).
Yup, in a world where modules are loaded conditionally (e.g. conditioned on the server-route; e.g. conditioned on From a DX POV, it's nice for the module metadata to be always-fresh/live. But then again...
In any case, regardless of when/how/if caching is added to the module list, we should probably phase-out the use of |
e699fbd
to
8cc06f4
Compare
Thanks @totten. I updated the test with half of your suggestion. |
c4ade9a
to
dbb704f
Compare
@totten actually nevermind the settings do all come back, I just needed to fix the logic. So now I've implemented your full suggestion to improve the test. |
This allows Angular modules with complex/expensive data to provide it with a callback, which will only be invoked if the module is actively loaded on the page. Normally, module settings are calculated on every page request, even if they are not needed.
dbb704f
to
aa882f9
Compare
@colemanw Glad to hear all settings come back. I did some TBH, I sort of feel that the Marking this "merge ready". @colemanw , if the example (#18749) is OK, and if you're OK punting the other bits, then let's merge. |
Overview
This allows Angular modules with complex/expensive data to provide it with a callback, which will only be invoked if the module is actively loaded on the page.
Docs updated at https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/862
Before
Angular modules can declare
settings
which are calculated on everyAngularLoader->load()
and then discarded if the module isn't needed on the current page. This can get expensive.After
Angular modules can declare
settingsFactory
which is only invoked when a module is needed on the current page.Comments
Currently (and I was a bit surprised by this) the full list of Angular modules is not cached anywhere. So
hook_civicrm_angularModules
runs on every page, and each module'ssettings
is loaded every time. That's a good thing from the POV of permissions (if say a module's settings are different for different users) but it's terrible for performance.Switching modules to use
settingsFactory
can save cycles in the short-term, and be even better in the long-term when we get around to caching the list of modules, as only the name of the callback will be cached, not the array of settings. This will also keep the per-user settings a possibility even with global caching of the modules.